Replace hapi with hono

[corrideat]

This closes #75 and closes #69

[Copilot]

Pull request overview

This PR migrates the backend HTTP layer from Hapi to Hono, updating server bootstrapping, routing/auth, and static file serving, and adds new Deno tests to validate endpoint behavior.

Changes:

• Replaced the Hapi server with a Hono app + @hono/node-server HTTP server adaptor.
• Ported REST endpoints, auth, and static asset serving from Hapi/Joi/Boom patterns to Hono + HTTPException.
• Added a new Deno test suite (helpers + read/write/KV/ZKPP/stateless tests) to cover the migrated routes.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/serve/server.ts	Replace Hapi server bootstrap with Hono + node-server adaptor and update WS version info payload.
src/serve/routes.ts	Port route definitions to Hono handlers, replace Boom/Joi validation with manual checks, and implement static file serving.
src/serve/auth.ts	Replace Hapi auth plugin with Hono middleware that sets credentials and authStrategy on context.
src/serve/dashboard-server.ts	Replace Hapi/Inert dashboard server with Hono + serveStatic.
src/serve/database.ts	Replace Boom exceptions with typed backend errors (NotFound/Gone/Conflict) for mapping at the HTTP layer.
src/serve/errors.ts	Add BackendErrorConflict to support conflict mapping.
src/serve/index.ts	Update shutdown log message to remove Hapi reference.
src/pin.ts	Use fullContractName when updating config keys and logging contract name.
src/eventsAfter.ts	Remove Boom-specific wording in comment.
deno.json	Add Hono imports and adjust test task flags.
deno.lock	Update dependency lock entries for Hono packages and other version shifts.
src/serve/routes-*.test.ts, src/serve/routes-test-helpers.ts	New route-level Deno tests covering the migrated endpoints.
build/*	Regenerated build artifacts reflecting the server/runtime changes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The upload size guard relies only on content-length, but for chunked uploads or missing/invalid content-length this becomes 0 and c.req.formData() will still buffer the entire body into memory. This is a regression from Hapi’s maxBytes and can allow large uploads/DoS. Consider enforcing a hard byte limit while reading the request body (or requiring a valid Content-Length and rejecting otherwise), and ensure the limit is applied before fully materializing formData().

[Copilot]

Buffer.from(await c.req.arrayBuffer()) reads the entire KV payload into memory with no size limit. Previously this endpoint had a maxBytes cap; without an explicit limit this can be abused for memory/CPU DoS. Add a request-size limit (e.g., reject when Content-Length exceeds the configured cap and/or enforce a streaming limit while reading the body).

Suggested change

	const payloadBuffer = Buffer.from(await c.req.arrayBuffer())
	const MAX_KV_PAYLOAD_BYTES = 1024 * 1024
	const readRequestBodyWithLimit = async (): Promise<Buffer> => {
	const contentLength = c.req.header('content-length')
	if (contentLength !== undefined) {
	const parsedLength = Number(contentLength)
	if (!Number.isFinite(parsedLength) || parsedLength < 0) {
	throw new HTTPException(400, { message: 'Invalid Content-Length' })
	}
	if (parsedLength > MAX_KV_PAYLOAD_BYTES) {
	throw new HTTPException(413, { message: 'Payload too large' })
	}
	}
	const body = c.req.raw.body
	if (!body) return Buffer.alloc(0)
	const reader = body.getReader()
	const chunks: Uint8Array[] = []
	let totalBytes = 0
	try {
	while (true) {
	const { done, value } = await reader.read()
	if (done) break
	if (!value) continue
	totalBytes += value.byteLength
	if (totalBytes > MAX_KV_PAYLOAD_BYTES) {
	throw new HTTPException(413, { message: 'Payload too large' })
	}
	chunks.push(value)
	}
	} finally {
	reader.releaseLock()
	}
	return Buffer.concat(chunks.map((chunk) => Buffer.from(chunk)), totalBytes)
	}
	const payloadBuffer = await readRequestBodyWithLimit()

[taoeffect]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.1

Click to expand review

---

Review generated using glm-5.1 via Z.AI. Comment /crush_fast to re-run.

[corrideat]

/crush_fast

---

AI review started.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

c.req.parseBody({ all: true }) returns values as string | File, but this handler casts data to string and passes it to createCID() / stores it directly. When clients upload a Blob (e.g. FormData.append('data', new Blob([...]))), data will be a File and this will fail. Handle File by reading its bytes/text before hashing and persisting (and keep the stored type consistent with what /file/:hash expects).

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.1

Click to expand review

Here is my review of the Hapi-to-Hono migration:

---

🔴 Issue 1: parseBooleanParam treats empty string as true

• Addressed
• Dismissed

File: src/serve/routes.ts:37

if (normalized === '' || normalized === 'true' || normalized === 'yes' || normalized === 'on' || normalized === '1') {
    return true
}

An empty keyOps query parameter (i.e., /eventsAfter/...?keyOps) will result in parseBooleanParam('') returning true. While this may match Joi's boolean() behavior for empty-string truthy values, the old code used Joi.boolean().truthy('1', 'on', 'yes').falsy('0', 'off', 'no') — which does not treat '' as true. This is a behavioral change that could cause keyOps to be unexpectedly enabled when a client sends ?keyOps without a value.

Suggested fix:

function parseBooleanParam (value: string | undefined): boolean {
  if (value === undefined || value === '') return false
  const normalized = value.toLowerCase()
  return normalized === 'true' || normalized === 'yes' || normalized === 'on' || normalized === '1'
}

---

🔴 Issue 2: closeDB() is called without await in migrate.ts

• Addressed
• Dismissed

File: src/migrate.ts:36

  } finally {
    closeDB()
  }

The closeDB function is async, but it is called without await in the finally block. If initDB succeeds and the exit(e) call in the catch block throws or exits the process, the closeDB() promise will be fire-and-forget. More critically, if the function continues past this point (e.g., the error path doesn't actually exit), the DB may not be fully closed before the migration proceeds with backendTo.

Suggested fix:

  } finally {
    await closeDB()
  }

---

🟡 Issue 3: migrate.ts always closes DB in finally even when it will be needed later

• Addressed
• Dismissed

File: src/migrate.ts:32-37

  try {
    await initDB({ skipDbPreloading: true })
  } catch (e) {
    console.error('Error setting up database')
    exit(e)
    throw e
  } finally {
    closeDB()
  }

The old code used SERVER_EXITING to clean up the DB connection at process end. The new code closes the DB eagerly in finally. But migrate.ts proceeds to use the DB for the "from" backend (reading all keys) after this block — specifically via sbp('chelonia.db/iterKeys') and sbp('chelonia.db/get') later in the function. After closeDB(), the overwritten chelonia.db/get selector will reference currentBackend which is null, causing a crash on currentBackend!.readData(key) (the ! non-null assertion will throw).

This appears to be a regression: the migration will fail when using a non-mem backend because the database is closed before the data can be read.

Suggested fix: Remove the finally { closeDB() } block and instead call closeDB() at the very end of the function, after all migration work is complete:

  try {
    await initDB({ skipDbPreloading: true })
  } catch (e) {
    console.error('Error setting up database')
    exit(e)
    throw e
  }

  // ... migration logic ...

  // Close after all work is done
  await closeDB()

---

🟡 Issue 4: getConnInfo will throw outside of @hono/node-server context

• Addressed
• Dismissed

File: src/serve/routes.ts:279-284

function getClientIP (c: Context): string {
  const headerIP = c.req.header('x-real-ip')
  if (headerIP) return headerIP
  try {
    const info = getConnInfo(c)
    return info.remote.address || 'unknown'
  } catch {
    return 'unknown'
  }
}

getConnInfo from @hono/node-server/conninfo requires that the server was started with @hono/node-server's serve() or createAdaptorServer(). The code does use createAdaptorServer in server.ts:437, so this should work in production. However, getConnInfo can also throw if the adapter middleware hasn't been installed. The try/catch handles this gracefully, so this is a minor concern — but worth noting that IP-based rate limiting silently falls back to 'unknown' if the adapter context is unavailable, which would mean all requests share the same rate limit bucket.

---

🟡 Issue 5: backend/server/stop selector calls itself recursively via stopServer()

• Addressed
• Dismissed

File: src/serve/server.ts:212-214

'backend/server/stop': async function () {
    await stopServer()
},

And then in src/serve/index.ts:99-107:

sbp('okTurtles.events/once', SERVER_EXITING, async () => {
    try {
      removeSignalHandlers()
      await sbp('chelonia.persistentActions/unload')
      await stopServer()

The stopServer() called from the SERVER_EXITING handler in index.ts calls stopServerImpl() from server.ts, which is also what stopServer() in index.ts delegates to. But there's also the backend/server/stop selector that calls stopServer() from server.ts directly. If any code calls sbp('backend/server/stop') AND the SERVER_EXITING handler also runs, stopServer() would be called twice. The isStopping guard in server.ts:643 prevents double-execution, so this isn't a crash — but it's worth noting the indirection is confusing.

---

🟡 Issue 6: parseArgs.ts calls yargsInstance.parse() synchronously, but main.ts awaits handlerState.postHandler() which may not be set yet

• Addressed
• Dismissed

File: src/parseArgs.ts:36-41 and src/main.ts:16

// parseArgs.ts
  yargsInstance.parse()
  return yargsInstance

// main.ts
  await handlerState.postHandler()

yargsInstance.parse() is synchronous in the sense that yargs processes argv immediately, but the command handler callback sets handlerState.postHandler. The issue is that parseArgs() returns the yargs instance, which is then passed to nconf.argv(). If yargs calls .parse() internally as well (which nconf.argv() may trigger), or if the parse behavior differs, postHandler might be called twice or not at all. The parse() call is explicit and synchronous, so the handler should run before parseArgs() returns — but this is a fragile pattern. The old code had a similar risk, but now the execution flow depends on the timing of yargsInstance.parse() vs the import of parseConfig.

---

⚪️ Issue 7: dashboard-server.ts uses serveStatic from hono/deno but the app is built for Node

• Addressed
• Dismissed

File: src/serve/dashboard-server.ts:4

import { serveStatic } from 'npm:hono/deno'

The dashboard server uses serveStatic from hono/deno (Deno-specific file serving) while the main server uses @hono/node-server. This works when running under Deno directly, but may be inconsistent when running as a compiled Node.js binary via deno compile. The serveStatic from hono/deno uses Deno.openFile/Deno.stat which may not work in all compilation targets.

---

⚪️ Issue 8: build.ts node-builtins plugin may shadow npm package names

• Addressed
• Dismissed

File: scripts/build.ts:31-39

    {
      name: 'node-builtins',
      setup (build) {
        build.onResolve({ filter: /^[a-zA-Z]/, namespace: 'file' }, ({ path }) => {
          if (nodeBuiltins.has(path)) {
            return { path: `node:${path}`, external: true }
          }
          return null
        })
      }
    },

This plugin runs before the skip plugin. The onResolve callback returns null for non-builtins, allowing other handlers to process them — this is correct. However, the filter: /^[a-zA-Z]/ regex is very broad and will be evaluated for every import that starts with a letter, adding a small overhead. This is a minor efficiency concern, not a bug.

---

⚪️ Issue 9: SERVER_INSTANCE is set to a Hono app but may be unused

• Addressed
• Dismissed

File: src/serve/server.ts:443

  sbp('okTurtles.data/set', SERVER_INSTANCE, currentApp)

SERVER_INSTANCE is stored but never read anywhere in the codebase (confirmed by search). The only references are its definition in instance-keys.ts and this line in server.ts. This is dead code that could be cleaned up.

---

⚪️ Issue 10: logger.ts default export is the raw pino instance but initializeLogger is now required

• Addressed
• Dismissed

File: src/serve/logger.ts

The file still export default logger (the raw pino instance). Meanwhile, routes.ts imports logger and uses it for logger.error(...). This is fine because the raw pino logger works independently of console replacement. But the pattern is now split: some code uses console.* (which requires initializeLogger() to be called), and other code imports logger directly. This dual logging path could cause confusion about which to use.

---

Review generated using glm-5.1 via Z.AI. Comment /crush_fast to re-run.

[corrideat]

Opus 4.7 (via Copilot)

Code Review

• Base: master
• Head: replace-hapi-with-hono (6b1317d)
• Date: 2026-04-20
• Scope: Replaces Hapi + Joi + Boom with Hono + Zod + HTTPException; introduces async start/stop server lifecycle. 24 non-build files changed across server, routes, auth, config, CLI bootstrapping, and tests.

Overall the migration is coherent and the Hono-based routing is cleaner. The main risks are lifecycle/shutdown semantics (SERVER_EXITING no longer emitted globally, selectors no longer locked) and a handful of small behavioral regressions compared to the Joi/Hapi originals.

---

1. 🔴 SERVER_EXITING is no longer emitted after a command completes

• Addressed
• Dismissed

src/main.ts used to wrap postHandler() in try { ... } finally { sbp('okTurtles.events/emit', SERVER_EXITING) }. The new code drops the try/finally entirely and just awaits handlerState.postHandler().

Consequences:

• Any listener registered on SERVER_EXITING (e.g. external cleanup, log flushing) will never fire on normal command completion.
• Cleanup has been moved piecemeal into individual commands: src/get.ts, src/upload.ts, src/eventsAfter.ts, and src/migrate.ts now each call closeDB() in their own finally. Commands that don't do this (e.g. hash, keygen, manifest, deploy, pin, verifySignature, latestState) no longer participate in the global shutdown event at all.

If this was intentional, add a comment in main.ts explaining the new contract (each command owns its own teardown). Otherwise, restore the emit:

try {
  await handlerState.postHandler()
} finally {
  sbp('okTurtles.events/emit', SERVER_EXITING)
}

2. 🔴 database.ts no longer locks its SBP selectors

• Addressed
• Dismissed

The previous implementation finished initDB with sbp('sbp/selectors/lock', [...]) for the chelonia.db/* and backend/db/* selectors. The new src/serve/database.ts removes those lock calls entirely.

This weakens the defense-in-depth guarantee that contract code (or any other module) cannot silently replace the database layer. If the reason for removal is to allow closeDB() + re-initDB() cycles (useful in tests and in stopServer/startServer), that's a legitimate trade-off — but please either:

• add a comment documenting why the selectors are intentionally left unlocked, or
• re-lock them at the end of initDB, and unlock them in closeDB before any replacement.

3. 🔴 X-Frame-Options middleware sets headers after await next() for streamed responses

• Addressed
• Dismissed

In src/serve/server.ts (≈ lines 420–430), the middleware does:

app.use('*', async (c, next) => {
  await next()
  c.header('X-Frame-Options', 'DENY')
})

For routes that construct a Response directly with a streaming body (notably /eventsAfter/* in routes.ts, which returns new Response(Readable.toWeb(stream), { headers })), the headers on that Response are already committed by the time this middleware resumes. c.header(...) after next() only modifies c.res, and whether Hono re-applies it to a user-constructed Response depends on Hono's internals — in practice this header is frequently lost for streamed replies.

The old Hapi onPreResponse hook ran before the response was flushed and was reliable. Verify by curling /eventsAfter/... and checking the response headers; if missing, merge the header into the Response construction explicitly or use c.res = new Response(c.res.body, { ...c.res, headers: ... }) pattern.

4. 🟡 parseBooleanParam does not match its own Joi-compat comment

• Addressed
• Dismissed

src/serve/routes.ts (≈ 24–32) documents itself as "Matches Joi.boolean().truthy('1','on','yes').falsy('0','off','no')", but:

• It returns true for empty string (so ?keyOps= becomes true). Joi's default would coerce this to false or reject.
• It returns false for any unrecognised value silently, whereas Joi would reject with a 400.

Either tighten the function to throw on unknown values (and let errorMapper turn it into a 400), or fix the comment to describe what it actually does. Suggested stricter version:

const TRUTHY = new Set(['1', 'on', 'yes', 'true'])
const FALSY  = new Set(['0', 'off', 'no', 'false', ''])
function parseBooleanParam (v: string | undefined, field: string): boolean {
  if (v === undefined) return false
  const s = v.toLowerCase()
  if (TRUTHY.has(s)) return true
  if (FALSY.has(s)) return false
  throw new HTTPException(400, { message: `Invalid boolean for ${field}` })
}

5. 🟡 zValidatorFormOrJson silently JSON-parses when Content-Type is missing/unexpected

• Addressed
• Dismissed

src/serve/routes.ts (≈ 119–149): if the request's Content-Type is not application/x-www-form-urlencoded, the validator falls through to c.req.json() regardless of whether Content-Type is application/json, missing, or something else entirely. The error surface ("Invalid request body") is also less informative than Joi's original per-field messages.

Consider:

• Requiring one of the two Content-Types explicitly and returning 415 otherwise.
• Including Zod's error.issues in the 400 response body (see errorMapper).

Also, for /zkpp/register/:name the Zod schema is z.union([{ b }, { r, s, sig, Eh }]). A payload like { b, r, s } (extra fields from the second branch) will match the first branch and the extras are silently dropped. The old Joi schema would have rejected. Consider .strict() on each branch.

6. 🟡 parseArgs() now eagerly .parse()s and is then passed to nconf.argv()

• Addressed
• Dismissed

src/parseArgs.ts calls yargsInstance.parse() at the end and returns the instance. src/parseConfig.ts:12 then does .argv(parseArgs()). nconf.argv(...) typically calls .argv on the yargs instance itself, which can cause yargs to re-run its command pipeline.

Confirm that the command's builder/handler is not executed twice (add a temporary console.log in one handler and run any command). If it is, switch to either:

• passing parseArgs().parsed.argv (the already-parsed result) to nconf, or
• not calling .parse() in parseArgs and letting nconf trigger it.

7. 🟡 migrate.ts uses backendTo!.close() in finally despite possible undefined

• Addressed
• Dismissed

src/migrate.ts (≈ line 155):

} finally {
  await Promise.all([backendTo!.close(), closeDB()])
}

If backendTo throws during init() before assignment (or if the try throws before it's constructed), the non-null assertion is a lie. In practice exit(error) is called first and short-circuits via Deno.exit(1), but that relies on non-local reasoning. Prefer:

await Promise.all([backendTo?.close(), closeDB()])

8. 🟡 /eventsAfter streaming: a DB error mid-stream produces a truncated 200

• Addressed
• Dismissed

src/serve/routes.ts (≈ 515) reads streamHeaders before the stream is consumed and wraps the Node stream via Readable.toWeb(stream). If the underlying DB read errors partway through, the HTTP status is already 200, the body aborts mid-flight, and the client has no structured way to distinguish this from a complete response.

This is arguably no worse than the Hapi implementation, but now would be a good time to at least log the stream error server-side:

stream.on('error', (err) => logger.error('eventsAfter stream error', err))

…and document the semantics in a comment.

9. 🟡 Rate-limiter cleanup on re-startServer() is correct but subtle

• Addressed
• Dismissed

installRateLimiterSelectorsOnce() in routes.ts (~234) and installServerSelectorsOnce() in server.ts use module-level "once" flags while the state they operate on (currentLimiters, currentApp, currentBackend) is module-let. On a second startServer() in the same process, registerRoutes first tears down the previous limiters (routes.ts:330), then builds new ones. The selector backend/server/stopRateLimiters re-reads currentLimiters at call-time, so it stays correct.

Not a bug, but:

• Add a short comment at the "once" installation sites explaining the invariant ("selectors close over the module-level mutable slot, which is overwritten in start/stop").
• Consider registering these selectors unconditionally on module load (they're idempotent closures) and removing the "once" flag + boolean.

10. 🟡 Dev-only POST /log assumes JSON without checking Content-Type

• Addressed
• Dismissed

src/serve/routes.ts (≈ 549–559) calls c.req.json() unconditionally. A non-JSON body throws an unhandled SyntaxError that surfaces as a 500 via errorMapper. Minor, since this route only exists in development, but wrapping with the standard zValidatorFormOrJson (or at least try/catch) would be more consistent with the rest of the file.

11. ⚪ Backwards-compat app proxy exposes undefined before startServer()

• Addressed
• Dismissed

src/serve/server.ts (≈ 701–708) exports a Proxy that forwards property access to currentApp. Before startServer() runs, currentApp is null and app.fetch (etc.) is undefined — silently, not a thrown error. The declared type is Hono, which is now a lie.

Either:

• Remove the export and migrate callers to startServer()/explicit access, or
• Throw in the proxy's get trap when currentApp === null:

get (_t, prop) {
  if (!currentApp) throw new Error('Server not started; call startServer() first')
  return Reflect.get(currentApp, prop, currentApp)
}

12. ⚪ Dashboard SPA fallback passes a noop next to serveStatic

• Addressed
• Dismissed

src/serve/dashboard-server.ts (≈ 33–42) invokes the serveStatic middleware manually with async () => {} as next and inspects the returned Response. This relies on Hono's serveStatic returning a Response when a file is found and calling next() (without returning one) when it isn't — an implementation detail that's not part of Hono's public contract.

A more idiomatic approach is to register serveStatic as middleware on the sub-app and then register a final catch-all app.get('*', ...) that serves index.html. That way the SPA fallback is just a normal route.

13. ⚪ safePathWithin is correct but should have direct tests

• Addressed
• Dismissed

src/serve/routes.ts (≈ 293–297) correctly resolves the target path and compares against base + path.sep to prevent /foo matching /foobar. Given that this is the critical guard on file-serving endpoints, add explicit unit tests in routes-test-helpers.ts / a new routes-static.test.ts for:

• ../../etc/passwd
• URL-encoded traversal (%2e%2e%2f...)
• Null-byte injection
• Absolute paths
• Paths whose resolved form equals base (should be allowed) vs basefoo (should not).

14. ⚪ Chunk-collection loop stops at first gap

• Addressed
• Dismissed

src/serve/routes.ts (≈ 720–725): for (let i = 0; ; i++) { if (!chunkFile) break } silently truncates at the first missing chunk index. If a client posts chunks 0, 1, 3, the server collects [0, 1] and may then fail schema validation with a confusing "missing chunk" error, or — worse — accept the upload missing chunk 2.

Either accept only contiguous chunks and validate count against an expected header, or explicitly detect gaps and return a 400 with a clear message.

---

Summary

• Critical (🔴): 3 — missing SERVER_EXITING emit, unlocked DB selectors, X-Frame-Options on streamed responses.
• Medium (🟡): 7 — boolean parsing, body validation loosening, yargs/nconf double-parse risk, migrate.ts non-null assertion, streaming error handling, rate-limiter install comments, dev /log body parsing.
• Minor (⚪): 4 — backwards-compat app proxy, dashboard SPA fallback pattern, missing safePathWithin tests, chunk-gap behavior.

No blockers for the migration itself; the Hono port is generally faithful. Recommended before merge: fix #1, #2, and #3; address #4 and #6 to avoid subtle behavioral regressions versus the Joi/Hapi baseline.

[devin-ai-integration]

Devin Review found 2 new potential issues.

View 33 additional findings in Devin Review.

[devin-ai-integration]

🟡 Auth: invalid shelter credentials in optional mode silently proceed as unauthenticated instead of returning 401

In the old Hapi-based auth scheme, when verifyShelterAuthorizationHeader threw an error (e.g., bad signature), the scheme returned Boom.unauthorized('Authentication failed', 'shelter') with a non-null message. In Hapi's optional auth mode, this still propagated as a 401 error. In the new extractShelter (src/serve/auth.ts:31-37), the catch block returns null, which causes authMiddleware in optional mode to set empty credentials and continue the request as unauthenticated. This means a request to /event with a present-but-invalid shelter authorization header (e.g., malformed signature) now silently proceeds without credentials instead of being rejected with 401, which could cause confusing downstream errors like 'This contract type requires ownership information' rather than a clear authentication failure.

Prompt for agents

In src/serve/auth.ts, the extractShelter function catches verification failures and returns null. When used with authMiddleware in 'optional' mode (as done in the POST /event route in routes.ts:339), this means a present-but-invalid shelter header is silently treated as unauthenticated rather than returning 401.

The old Hapi behavior distinguished between 'no credentials' (Boom.unauthorized(null, scheme) -> proceed in optional mode) and 'failed auth' (Boom.unauthorized('message', scheme) -> 401 even in optional mode).

To preserve the old behavior, extractShelter should throw or signal differently when the header IS present and has the correct prefix but verification fails. One approach: have the extractor return a sentinel value (not null) to indicate 'auth attempted but failed', and have authMiddleware reject with 401 in that case regardless of mode. Another approach: have extractShelter throw the HTTPException(401) directly when verification fails (not when the prefix doesn't match).

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🟡 POST /event body is read with c.req.text() after zValidator('header') may have already consumed it in some edge cases

The /event route (src/serve/routes.ts:331-339) chains zValidator('header'), bodyLimit(), authMiddleware(), then the handler reads the body with c.req.text(). While zValidator('header') and authMiddleware don't consume the body, there's a more fundamental issue: the zValidator('header') middleware does not use .passthrough() or leave unknown properties alone by default in all Zod v4 configurations. The schema without .strict() strips unknown keys in the output, but importantly the schema validates ALL headers including standard ones like content-type. Since content-type may have values like application/json; charset=utf-8, and the schema only defines optional custom headers, the unknown standard headers are silently stripped from c.req.valid('header') but won't cause errors. However, the shelter-namespace-registration header is validated with nameSchema which has a strict regex — if a request includes this header with an invalid value it will get a 400 from the zod validator rather than being handled more specifically by the route handler. This is actually correct behavior, but worth noting it differs from the old Hapi approach which validated headers only after auth.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[corrideat]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.1

Click to expand review

Now let me check some cross-references and look at the test files for potential issues.Here is my review of the "Replace hapi with hono" changes.

---

Issue 1 — 🔴 import.meta.initDbOnce missing from type definition

• Addressed
• Dismissed

src/types/build.d.ts does not include initDbOnce, but src/serve/database.ts references it in two places:

// src/serve/database.ts:281
if ((import.meta as ImportMeta).initDbOnce) {

// src/serve/database.ts:356
if (!(import.meta as ImportMeta).initDbOnce) {

The type definition at src/types/build.d.ts:1-5 only declares:

interface ImportMeta {
  VERSION: string,
  ownerSizeTotalWorker?: string,
  creditsWorker?: string
}

The as ImportMeta cast masks the problem at compile time but is misleading — the cast asserts the value is ImportMeta, but ImportMeta doesn't have initDbOnce either, so the property access should be a type error. It likely only compiles because import.meta in Deno is broadly typed. Add initDbOnce?: boolean to the interface and remove the casts.

---

Issue 2 — 🟡 SERVER_INSTANCE is dead code

• Addressed
• Dismissed

src/serve/server.ts:443 stores the Hono app into SBP:

sbp('okTurtles.data/set', SERVER_INSTANCE, currentApp)

But SERVER_INSTANCE (@instance/server) is never read anywhere in the codebase. It was used under Hapi to retrieve the server instance for route handling; with Hono this pattern is no longer needed. The import and the sbp('okTurtles.data/set', ...) call on server.ts:443 are dead code and should be removed to avoid confusion.

---

Issue 3 — 🟡 getConnInfo import but risky fallback in getClientIP

• Addressed
• Dismissed

src/serve/routes.ts:233-242:

export function getClientIP (c: Context): string {
  const headerIP = c.req.header('x-real-ip')
  if (headerIP) return headerIP
  try {
    const info = getConnInfo(c)
    return info.remote.address || 'unknown'
  } catch {
    return 'unknown'
  }
}

The x-real-ip header is trusted without validation. The code comment at routes.ts:343 even warns about this:

IMPORTANT: IT IS A REQUIREMENT THAT ANY PROXY SERVERS (E.G. nginx) IN FRONT OF US SET THE X-Real-IP HEADER! OTHERWISE THIS IS EASILY SPOOFED!

This is a pre-existing concern, but the migration is an opportunity to improve it. At minimum, getConnInfo should be the primary source with x-real-ip as fallback (not the other way around), or the header should only be trusted if a known proxy configuration is set.

---

Issue 4 — 🟡 Potential race condition in closeDB() re-entrancy guard

• Addressed
• Dismissed

src/serve/database.ts:329-358:

export async function closeDB (): Promise<void> {
  if (isClosing) return
  isClosing = true
  try {
    // ... async operations ...
  } finally {
    isClosing = false
  }
}

The isClosing flag is checked synchronously and set to true, but if two callers invoke closeDB() concurrently, the second call silently returns undefined (resolves immediately) without waiting for the first call's cleanup to finish. This could lead to callers thinking the DB is closed when it's still shutting down. Consider using a promise-based guard:

let closePromise: Promise<void> | null = null
export async function closeDB (): Promise<void> {
  if (closePromise) return closePromise
  closePromise = (async () => {
    // ... actual close logic ...
  })()
  try {
    await closePromise
  } finally {
    closePromise = null
  }
}

---

Issue 5 — 🟡 initedDB state not fully reset in closeDB

• Addressed
• Dismissed

src/serve/database.ts:349-357:

export async function closeDB (): Promise<void> {
  // ...
  if (!(import.meta as ImportMeta).initDbOnce) {
    initedDB = false
  }
}

When initDbOnce is true (the build output), initedDB is never reset. This means after closeDB(), a subsequent initDB() call will see initedDB as 'preloaded' (or true) and skip re-initialization — but the backend and cache have been nulled out. Any call to chelonia.db/get, etc. will crash with a null dereference on currentBackend!. This is likely intentional for the build (single-init), but it's fragile. If closeDB() is called while initDbOnce is true, the overwritten selectors should either be restored to safe no-ops or the code should guard against currentBackend being null inside the selectors.

---

Issue 6 — 🟡 dashboard-server.ts — Deno.serve return value not stored for graceful shutdown

• Addressed
• Dismissed

src/serve/dashboard-server.ts:42-44:

await new Promise<void>((resolve) => {
  Deno.serve({ port, hostname: host, onListen: () => resolve() }, app.fetch)
})

The Deno.Server returned by Deno.serve() is not stored anywhere. This means there is no way to gracefully shut down the dashboard server (e.g., on SIGTERM). The main server has stopServer() but the dashboard has no equivalent. This was also a limitation with the old Hapi code, but is worth noting.

---

Issue 7 — ⚪️ authMiddleware runs extractor for all strategies before choosing

• Addressed
• Dismissed

src/serve/auth.ts:50-63:

return async (c, next) => {
  for (const strategy of strategyList) {
    const extractor = extractors[strategy]
    if (!extractor) throw new Error(`Unknown auth strategy: ${strategy}`)
    const credentials = extractor(c)
    if (credentials) {
      c.set('credentials', credentials)
      c.set('authStrategy', strategy)
      return next()
    }
  }

The extractShelter function at line 29 calls verifyShelterAuthorizationHeader(authorization) which likely does cryptographic verification. When multiple strategies are passed (e.g., ['chel-shelter', 'chel-bearer'] at routes.ts:802), the shelter extractor runs first and performs expensive crypto even if the request is a simple bearer token. Consider checking the Authorization header prefix before invoking the extractor to short-circuit:

const credentials = authorization.startsWith(strategyPrefix[strategy])
  ? extractor(c)
  : null

---

Issue 8 — ⚪️ main.ts — handlerState.postHandler defaults to no-op, causing silent failure

• Addressed
• Dismissed

src/main.ts:16:

await handlerState.postHandler()

src/parseArgs.ts:9:

const handlerState: { postHandler: () => void | Promise<void> } = {
  postHandler: () => {}
}

If no command is matched or if parseArgs fails to set postHandler, the default no-op function runs silently — the process exits with code 0 having done nothing. The old code used postHandler (a module-level variable) which had the same issue, but the new handlerState object wrapper doesn't improve this. Consider throwing or logging a warning if postHandler is still the default no-op when called.

---

Issue 9 — ⚪️ routes.ts — z.stringbool() may not exist in Zod v4

• Addressed
• Dismissed

src/serve/routes.ts:468:

keyOps: z.stringbool().optional()

The deno.json pins zod@4.0.5. In Zod v4, the stringbool() method was introduced as a coerce utility, but its behavior may differ from what's expected here (it's designed for form data coercion). Since this is a query parameter, it would be safer to use z.enum(['true', 'false']).optional().transform(...) or z.coerce.boolean().optional() to be explicit about the expected behavior.

---

Issue 10 — ⚪️ migrate.ts — Indentation added without functional change; backendTo.close() removed from error paths

• Addressed
• Dismissed

In the diff for src/migrate.ts, the entire migration loop body was re-indented inside a try/finally block, which is an improvement for cleanup. However, the individual catch blocks that previously called await backendTo.close() before exit(1) no longer do so:

// Old:
} catch (e) {
  reportStatus()
  console.error(`Error reading from source database key '${key}'`, e)
  await backendTo.close()
  exit(1)
  throw e
}

// New:
} catch (e) {
  reportStatus()
  console.error(`Error reading from source database key '${key}'`, e)
  exit(1)
  throw e
}

The finally block now handles closing: await Promise.all([backendTo?.close(), closeDB()]). Since exit() is called in the catch, and exit uses process.exit internally, the finally block may not actually execute. This is likely fine since exit() already handles process termination, but it's worth confirming that exit() in migrate.ts's context allows the finally to run.

---

Issue 11 — ⚪️ build.ts — node-builtins plugin may match non-Node imports

• Addressed
• Dismissed

scripts/build.ts:33-41:

{
  name: 'node-builtins',
  setup (build) {
    build.onResolve({ filter: /^[0-9a-zA-Z_/]+$/, namespace: 'file' }, ({ path }) => {
      if (nodeBuiltins.has(path)) {
        return { path: `node:${path}`, external: true }
      }
      return null
    })
  }
}

The filter /^[0-9a-zA-Z_/]+$/ matches any bare import like ./some/path or ../foo/bar. Although return null is correct for non-builtins, this plugin adds overhead to every non-prefixed import resolution. The filter should be narrowed to only match potential Node builtins (e.g., single-word paths without /), like /^[a-zA-Z][a-zA-Z0-9]*$/.The overall migration from Hapi to Hono is well-structured. The route definitions map cleanly, the auth middleware is a good translation from Hapi's auth schemes, and the server lifecycle management (start/stop) is properly reorganized. The most important issues to address are #1 (missing type) and #5 (potential null dereference after closeDB with initDbOnce).

---

Review generated using glm-5.1 via Z.AI. Comment /crush_fast to re-run.

[taoeffect]

Could also real quick benchmark the current server vs this one to verify that in fact Hono is faster than Hapi?

GI has a backend unit test that posts events. You could benchmark posting and reading events.