Add endpoint tests

[corrideat]

Endpoint tests

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 6 additional findings.

[corrideat]

/crush_fast

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

Let me examine the relevant source files to understand the codebase context better.Now let me check a few more specific details before writing my review.Now let me verify one more critical thing about how the module caching works with the test setup:Now I have enough context. Here is my review:

---

Code Review: Add Endpoint Tests

Issue #1: 🔴 Tests cannot restart server — startTestServer() hangs after first test

• Addressed
• Dismissed

This is the most critical issue. All five test files call startTestServer() and stopTestServer(), but Deno runs all test files in the same process with shared module caching. The server startup logic lives in a module-level IIFE in server.ts:513-582 that only executes once:

// src/serve/server.ts:513-582
;(async function () {
  await initDB()
  // ...
  await hapi.start()
  sbp('okTurtles.events/emit', SERVER_RUNNING, hapi)
})()

Because import('./server.ts') is cached by the ES module loader, this IIFE runs exactly once per process. The SERVER_RUNNING event is emitted once. After stopTestServer() calls hapi.stop(), any subsequent startTestServer() call will hang forever at:

// src/serve/routes-test-helpers.ts:138-148
const serverAddress = await new Promise<string>((resolve, reject) => {
  const unregister = sbp('okTurtles.events/once', SERVER_RUNNING, function (hapi: { info: { uri: string } }) {
    resolve(hapi.info.uri)
  })
  import('./index.ts').then(({ default: start }) => {
    return start()
  }).catch((e) => {
    unregister()
    reject(e)
  })
})

The import('./index.ts') resolves with the cached module, start() re-registers a SERVER_RUNNING listener via sbp('okTurtles.events/on', ...), but import('./server.ts') inside start() also resolves from cache with no re-execution. The SERVER_RUNNING event is never emitted again, so the promise never resolves.

Additionally, stopTestServer() will also break on subsequent calls because the SERVER_EXITING handler in index.ts:42 is registered with sbp('okTurtles.events/once') and is consumed by the first stop — there is no handler for subsequent SERVER_EXITING emissions.

Suggested fix: Restructure startTestServer to handle restart. Instead of relying on import() to trigger server initialization, extract the server startup into a callable function and use Hapi's start()/stop() lifecycle directly:

// In routes-test-helpers.ts
let serverStarted = false

export async function startTestServer (): Promise<string> {
  // ... nconf setup ...

  if (!serverStarted) {
    // First time: import the module (triggers IIFE)
    const serverAddress = await new Promise<string>((resolve, reject) => {
      const unregister = sbp('okTurtles.events/once', SERVER_RUNNING, function (hapi: { info: { uri: string } }) {
        resolve(hapi.info.uri)
      })
      import('./index.ts').then(({ default: start }) => start()).catch((e) => {
        unregister()
        reject(e)
      })
    })
    serverStarted = true
    return serverAddress
  }
  // Subsequent: restart the already-created Hapi server
  const hapi = sbp('okTurtles.data/get', SERVER_INSTANCE)
  await hapi.start()
  return hapi.info.uri
}

Alternatively, combine all five test files into a single Deno.test block, or add a flag/guard in server.ts that re-emits SERVER_RUNNING if the Hapi server is restarted.

---

Issue #2: 🟡 --sloppy-imports flag suppresses import resolution warnings

• Addressed
• Dismissed

// deno.json:8
-    "test": "deno task lint && deno test --unstable-worker-options ..."
+    "test": "deno task lint && deno test --sloppy-imports --unstable-worker-options ..."

The --sloppy-imports flag tells Deno to silently resolve imports that are missing file extensions. This masks actual import problems — if an import path is wrong or an extension is missing, Deno will silently try to guess the correct file instead of reporting an error. This should be a temporary workaround at best. If it was added because some dependency lacks extensions, the root cause should be fixed instead.

---

Issue #3: 🟡 Redundant sbp('backend/server/stop') in stopTestServer

• Addressed
• Dismissed

// src/serve/routes-test-helpers.ts:153-163
export async function stopTestServer (): Promise<void> {
  await new Promise<void>((resolve) => {
    sbp('okTurtles.events/once', SERVER_EXITING, () => {
      sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
        resolve()
      })
    })
    sbp('okTurtles.events/emit', SERVER_EXITING)
  })
  await sbp('backend/server/stop')  // ← Already called inside SERVER_EXITING handler in index.ts:42-63
}

The SERVER_EXITING handler in src/serve/index.ts:42-63 already calls sbp('backend/server/stop') (which calls hapi.stop()). The explicit call in stopTestServer calls hapi.stop() a second time. While Hapi handles double-stop gracefully, this is confusing and suggests the author wasn't aware of the duplication. Remove the extra call:

export async function stopTestServer (): Promise<void> {
  await new Promise<void>((resolve) => {
    sbp('okTurtles.events/once', SERVER_EXITING, () => {
      sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
        resolve()
      })
    })
    sbp('okTurtles.events/emit', SERVER_EXITING)
  })
}

---

Issue #4: 🟡 Unused _contractID parameter in buildSignedKvPayload

• Addressed
• Dismissed

// src/serve/routes-test-helpers.ts:19-31
export function buildSignedKvPayload (
  _contractID: string,   // ← never used in the function body
  key: string,
  height: number,
  data: unknown,
  SAK: ReturnType<typeof keygen>
) {
  const SAKid = keyId(SAK)
  const heightStr = String(height)
  const serializedMessage = JSON.stringify(data)
  const additionalData = key + heightStr
  const sig = sign(SAK, blake32Hash(blake32Hash(additionalData) + blake32Hash(serializedMessage)))
  // ...

The _contractID parameter is never used. The underscore prefix indicates this was known, but every call site passes owner.contractID as the first argument, creating the misleading impression it's used in signing. If the server's KV endpoint expects the contractID to be part of the signed payload, this is a correctness bug — the signature would be missing the contractID binding. If it's truly unused, remove it from the signature and all call sites.

---

Issue #5: ⚪️ ImportMeta cast in server.ts appears unnecessary

• Addressed
• Dismissed

// src/serve/server.ts:46-51
-  : createWorker(join(import.meta.dirname || '.', import.meta.workerDir || '.', 'ownerSizeTotalWorker.js'))
+  : createWorker(join(import.meta.dirname || '.', (import.meta as ImportMeta).workerDir || '.', 'ownerSizeTotalWorker.js'))

The file src/types/build.d.ts already augments the global ImportMeta interface:

// src/types/build.d.ts
interface ImportMeta {
  VERSION: string
  workerDir?: string
}

Since this is a global interface declaration, import.meta.workerDir should be typed correctly without an explicit cast. If the cast was added because TypeScript wasn't picking up the augmentation, the root cause (likely a missing reference to build.d.ts in the TypeScript configuration) should be fixed instead.

---

Issue #6: ⚪️ sanitizeResources: false / sanitizeOps: false disables leak detection across all test files

• Addressed
• Dismissed

All five test files set:

sanitizeResources: false,
sanitizeOps: false,

This disables Deno's built-in resource and async operation leak detection. While this is commonly needed for integration tests that hold open server sockets, it means resource leaks in the server code won't be caught. Consider scoping these to only the test cases that need them, or ensuring a dedicated resource-leak test exists separately that doesn't disable these sanitizers.

---

Issue #7: ⚪️ Module-level constants in routes.ts are frozen on first import

• Addressed
• Dismissed

// src/serve/routes.ts:40-45
const FILE_UPLOAD_MAX_BYTES = nconf.get('server:fileUploadMaxBytes') || 30 * MEGABYTE
const SIGNUP_LIMIT_MIN = nconf.get('server:signup:limit:minute') || 2
const SIGNUP_LIMIT_HOUR = nconf.get('server:signup:limit:hour') || 10
const SIGNUP_LIMIT_DAY = nconf.get('server:signup:limit:day') || 50
const SIGNUP_LIMIT_DISABLED = process.env.NODE_ENV !== 'production' || nconf.get('server:signup:limit:disabled')
const ARCHIVE_MODE = nconf.get('server:archiveMode')

These are evaluated once when routes.ts is first imported. The startTestServer helper sets nconf values after the module may already be loaded (the first import('./server.ts') triggers import('./routes.ts') at server.ts:578). If any of these constants are needed in test assertions, they'll reflect whatever nconf contained at first import time — which may not match the test's intended configuration. This isn't a test bug per se (the test values happen to work), but it's fragile and worth being aware of.

---

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

[corrideat]

/crush_fast

---

AI review started.

[corrideat]

/crush_fast

---

AI review started.

[corrideat]

The issues the LLMs point out about server start / stop make sense, but for some reason tests are currently working in spite of those issues.

Since the purpose of this PR is to get tests for the API to ensure that changing the backend doesn't break the API, I'd say we can ignore those issues for the time being. We should later address this by bootstrapping the server in a manner that's easier to maintain and reason about, taking advantage of top-level await, which we can now use.

This PR currently looks good to merge, since tests pass and the logic hasn't been changed much, other than for some minor bugfixes.

The important part when reviewing this is ensuring that those few places that change application logic are correct.

[corrideat]

/crush_fast

---

AI review started.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

Advanced AI Review

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

Click to expand review

Let me examine the relevant source files for context before providing my review.Now let me look at the sbp/selectors/fn check and the rate limiter private API access:Now I have all the context needed. Let me compile my review.

Code Review

Issue 1: 🔴 Tests will hang indefinitely on second startTestServer() call

• Addressed
• Dismissed

Each of the 5 new test files calls startTestServer() → stopTestServer(). Because Deno runs all test files sequentially in a single process, and ES modules are cached after first evaluation, only the first test file to execute will successfully start the server.

The startTestServer function in src/serve/routes-test-helpers.ts:132-142 calls import('./index.ts'), which triggers import('./server.ts'). The server initialization lives in a module-level IIFE at src/serve/server.ts:520-589:

;(async function () {
  // ...
  await hapi.start()
  sbp('okTurtles.events/emit', SERVER_RUNNING, hapi)
})()

This IIFE runs exactly once (on first module evaluation). On the second startTestServer() call, import('./server.ts') returns the cached module — the IIFE does not re-execute, hapi.start() is never called again, and SERVER_RUNNING is never re-emitted. The once handler registered at routes-test-helpers.ts:133 will never fire.

Since Deno has no default test timeout (no --timeout flag in the test command at deno.json:8, and no timeout property in any Deno.test() call), the process will hang forever.

Suggestion: Either:

1. Run each test file in a separate process using --parallel or separate deno test invocations, or
2. Restructure the tests to use a single startTestServer()/stopTestServer() pair wrapping all test steps, or
3. Extract server startup into a restartable function (not a module-level IIFE).

---

Issue 2: 🔴 Rate limiters access private interval property via as unknown as cast

• Addressed
• Dismissed

src/serve/routes.ts:78-80:

clearInterval((limiterPerMinute as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerHour as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerDay as unknown as { interval: ReturnType<typeof setInterval> }).interval)

This accesses an undocumented private property of the Bottleneck library's Group class. The comment acknowledges this ("needs to be done using a private API because disconnect() isn't enough"), but this is fragile:

• The property name interval could change in any Bottleneck minor/patch update without warning.
• If the property doesn't exist or is undefined, clearInterval(undefined) is a silent no-op, meaning timers would leak silently.

Suggestion: Add a guard:

for (const limiter of [limiterPerMinute, limiterPerHour, limiterPerDay]) {
  limiter.disconnect()
  const internal = (limiter as unknown as { interval?: ReturnType<typeof setInterval> }).interval
  if (internal != null) clearInterval(internal)
}

---

Issue 3: 🟡 stopTestServer has no timeout — can hang indefinitely

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:147-155:

export async function stopTestServer (): Promise<void> {
  await new Promise<void>((resolve) => {
    sbp('okTurtles.events/once', SERVER_EXITING, () => {
      sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
        resolve()
      })
    })
    sbp('okTurtles.events/emit', SERVER_EXITING)
  })
}

If the shutdown chain hangs (e.g., pubsub.close() never fires its 'close' event, or hapi.stop() never resolves), this promise never resolves. Combined with the lack of a Deno test timeout, this would hang the entire test runner.

Suggestion: Add a safety timeout:

export async function stopTestServer (): Promise<void> {
  await Promise.race([
    new Promise<void>((resolve) => {
      sbp('okTurtles.events/once', SERVER_EXITING, () => {
        sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
          resolve()
        })
      })
      sbp('okTurtles.events/emit', SERVER_EXITING)
    }),
    new Promise<void>((resolve) => setTimeout(() => {
      console.error('stopTestServer timed out after 10s')
      resolve()
    }, 10_000))
  ])
}

---

Issue 4: 🟡 startTestServer has no timeout — can hang indefinitely

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:132-142:

const serverAddress = await new Promise<string>((resolve, reject) => {
  const unregister = sbp('okTurtles.events/once', SERVER_RUNNING, function (hapi: { info: { uri: string } }) {
    resolve(hapi.info.uri)
  })
  import('./index.ts').then(({ default: start }) => {
    return start()
  }).catch((e) => {
    unregister()
    reject(e)
  })
})

If the server starts but SERVER_RUNNING is never emitted (e.g., initialization fails silently), or on a second call (Issue 1), this promise hangs forever with no timeout.

Suggestion: Add a timeout:

const serverAddress = await new Promise<string>((resolve, reject) => {
  const timeout = setTimeout(() => {
    unregister()
    reject(new Error('startTestServer timed out after 30s'))
  }, 30_000)
  const unregister = sbp('okTurtles.events/once', SERVER_RUNNING, function (hapi: { info: { uri: string } }) {
    clearTimeout(timeout)
    resolve(hapi.info.uri)
  })
  import('./index.ts').then(({ default: start }) => {
    return start()
  }).catch((e) => {
    clearTimeout(timeout)
    unregister()
    reject(e)
  })
})

---

Issue 5: 🟡 chelonia/rootState leaks across test files

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:71-94 — createTestIdentity() mutates shared global state:

export function createTestIdentity () {
  // ...
  const rootState = sbp('chelonia/rootState')
  rootState[contractID] = {
    _vm: {
      authorizedKeys: {
        [SAKid]: { /* ... */ }
      }
    }
  }
  rootState.contracts = rootState.contracts || Object.create(null)
  rootState.contracts[contractID] = {
    type: 'gi.contracts/identity',
    HEAD: createCID(contractData + '-head', multicodes.SHELTER_CONTRACT_DATA),
    height: 0
  }
  return { contractID, SAK, SAKid }
}

stopTestServer() does not clean up chelonia/rootState. If tests were to run successfully in sequence (after fixing Issue 1), identities from earlier tests would persist into later tests, potentially causing false positives or unexpected interactions.

Suggestion: Either clean up rootState in stopTestServer(), or track created identities and clean them up after each test file's finally block.

---

Issue 6: 🟡 removeSignalHandlers is exported but never imported externally

• Addressed
• Dismissed

src/serve/index.ts:114-118:

export function removeSignalHandlers () {
  for (const [signal, handler] of signalHandlers) {
    process.removeListener(signal, handler)
  }
  signalHandlers.length = 0
}

This function is exported and called only within the same file (at line 48). While exporting it makes sense for potential external use (e.g., tests), if it's intentionally exported for the test helpers, it should be documented. If not needed externally, the export is unnecessary but harmless.

Suggestion: Either add a comment explaining why it's exported, or remove the export if it's not needed by external code. Currently it is not imported by any test helper.

---

Issue 7: ⚪️ buildShelterAuthHeader nonce is only 15 bytes

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:99-104:

export function buildShelterAuthHeader (contractID: string, SAK: ReturnType<typeof keygen>) {
  const nonceBytes = new Uint8Array(15)
  crypto.getRandomValues(nonceBytes)
  const data = `${contractID} ${Date.now()}.${Buffer.from(nonceBytes).toString('base64')}`
  return `shelter ${data}.${sign(SAK, data)}`
}

15 bytes provides 120 bits of entropy which is sufficient for test purposes. However, the choice of 15 bytes (rather than a more standard 16 or 32) is unusual. When base64-encoded, 15 bytes produces 20 characters with one trailing = pad. Using 16 bytes would produce a clean 24-character base64 with no padding. This is cosmetic and not a security concern.

---

Issue 8: ⚪️ process.on('uncaughtException') and process.on('unhandledRejection') handlers are never removed

• Addressed
• Dismissed

src/serve/index.ts:67-75:

process.on('uncaughtException', (err: unknown) => {
  console.error(err, 'uncaughtException')
  process.exit(1)
})
process.on('unhandledRejection', (reason: unknown) => {
  console.error(reason, 'unhandledRejection')
  process.exit(1)
})

These are registered at module level and never cleaned up, even after removeSignalHandlers() is called. In the server's normal lifecycle this doesn't matter (the process exits anyway), but in test environments where the module is loaded once and the process doesn't exit between tests, these handlers persist and could interfere with test frameworks that expect to handle uncaught exceptions themselves.

Suggestion: If this becomes a problem for testing, consider registering these conditionally (e.g., only when not in test mode) or adding cleanup to removeSignalHandlers().

---

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

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[corrideat]

/crush_fast

---

AI review started.

[taoeffect]

/crush_fast

(Trying one last time)

---

AI review started.

[taoeffect]

Review from GLM-5.1 locally:

Details

Code Review

Base: master
Head: pr (8b4b325)
Date: 2026-04-17

---

This PR adds comprehensive HTTP endpoint tests covering stateless endpoints, state-dependent reads, write operations, KV store, and ZKPP (Zero-Knowledge Password Proof) flows. It also improves server shutdown cleanup (rate limiters, workers, heartbeat interval) and refactors worker path injection from a single workerDir to individual import.meta properties.

The test coverage is thorough — ~60 test cases across 5 test files exercising happy paths, error cases, auth flows, content-type validation, cache headers, and full cryptographic ZKPP registration/challenge flows.

1. 🟡 Fragile private API access for Bottleneck cleanup

• Addressed
• Dismissed

src/serve/routes.ts:78-80 accesses a private interval property on Bottleneck Group instances:

clearInterval((limiterPerMinute as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerHour as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerDay as unknown as { interval: ReturnType<typeof setInterval> }).interval)

The comment acknowledges this, but it's worth noting that this will silently break if Bottleneck renames or removes the interval property in an update. Consider wrapping in a try/catch so a future Bottleneck upgrade doesn't crash shutdown:

try {
  clearInterval((limiterPerMinute as unknown as { interval?: ReturnType<typeof setInterval> }).interval)
  clearInterval((limiterPerHour as unknown as { interval?: ReturnType<typeof setInterval> }).interval)
  clearInterval((limiterPerDay as unknown as { interval?: ReturnType<typeof setInterval> }).interval)
} catch {
  // Private API may have changed across Bottleneck versions
}

2. ⚪️ Inconsistent validation order in ZKPP full flow tests

• Addressed
• Dismissed

The two ZKPP full flow tests validate the encrypted salt response in different orders:

routes-zkpp.test.ts:180-181 (JSON flow) — regex check before empty check:

if (/[^\dA-Za-z_-]/.test(encryptedSalt)) throw new Error('Invalid characters in encrypted salt')
if (!encryptedSalt) throw new Error('Expected encrypted salt response')

routes-zkpp.test.ts:306-307 (form-urlencoded flow) — empty check before regex check:

if (!encryptedSalt) throw new Error('Expected encrypted salt response')
if (/[^\da-zA-Z_-]/.test(encryptedSalt)) throw new Error('Unexpected characters in encrypted salt')

The empty check should come first since an empty string would pass the regex check (no invalid characters to find). The first test has the order reversed. Additionally, the regex patterns use different casing (A-Za-z vs a-zA-Z) — functionally identical but inconsistent.

3. ⚪️ Duplicated ZKPP full flow test logic

• Addressed
• Dismissed

routes-zkpp.test.ts — The "ZKPP full flow: register, challenge, get contract salt" test (line 127) and the "ZKPP full flow with form-urlencoded registration" test (line 250) are ~70 lines of nearly identical logic, differing only in Content-Type and body serialization. Consider extracting a shared helper:

async function zkppFullFlow (baseURL: string, contentType: 'json' | 'form') {
  // shared logic with parameterized content type
}

This would reduce duplication and ensure both code paths stay in sync as the protocol evolves.

4. ⚪️ buildSignedKvPayload accepts unused _contractID parameter

• Addressed
• Dismissed

routes-test-helpers.ts:24 — The first parameter _contractID is accepted but never used in the signing logic. The actual signature only covers key + height. While the underscore prefix convention marks it as intentionally unused, and the tests pass because the server's chelonia/parseEncryptedOrUnencryptedDetachedMessage receives contractID separately, this could mislead future test authors into thinking the contractID is part of the signed payload. Consider removing the parameter or adding a brief comment explaining why it's excluded.

export function buildSignedKvPayload (
  _contractID: string, // intentionally unused: contractID is verified separately by the server
  key: string,
  ...
)

[github-actions]

Advanced AI Review

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

Click to expand review

Now let me verify one more thing about the pingIntervalID type change in pubsub.ts:Now I have enough context to write a comprehensive review. Let me check the sbp('sbp/selectors/fn', ...) guard pattern:Now I have all the context I need. Let me write the review.

Code Review

Issue 1: Tests will deadlock if run in parallel — startTestServer is not idempotent

• Addressed
• Dismissed

🟡 Medium importance, high confidence

The test command in deno.json:8 does not use --sequential:

"test": "deno task lint && deno test --unstable-worker-options ..."

Deno 2.x runs Deno.test blocks in parallel by default. Five test files each call startTestServer() which does:

// routes-test-helpers.ts:136
import('./index.ts').then(({ default: start }) => {
  return start()
})

After the first call, ./index.ts and ./server.ts are cached modules. The top-level IIFE in server.ts (line 520) only runs once. A second call to start() will register a SERVER_RUNNING listener, but that event was already emitted and won't fire again — the second test hangs forever.

Similarly, the SERVER_EXITING handler in index.ts:42 is registered with once, so only the first stopTestServer() call actually triggers pubsub close + hapi stop + worker termination. A second stop call resolves without cleanup.

Recommendation: Either add --sequential to the test command, or refactor startTestServer to detect an already-running server and return the existing address:

let cachedServerAddress: string | null = null

export async function startTestServer(): Promise<string> {
  if (cachedServerAddress) return cachedServerAddress
  // ... existing setup ...
  cachedServerAddress = serverAddress
  return serverAddress
}

export async function stopTestServer(): Promise<void> {
  cachedServerAddress = null
  // ... existing teardown ...
}

---

Issue 2: pushHeartbeatIntervalID declared after code that references it by closure

• Addressed
• Dismissed

🟡 Medium importance, high confidence

In server.ts, the 'backend/server/stop' selector (line 249–259) references pushHeartbeatIntervalID via closure, but the variable is declared at line 592:

// line 249
'backend/server/stop': async function () {
    clearInterval(pushHeartbeatIntervalID)  // references line 592's variable
    ...
}

// ... ~340 lines later ...

// line 592
let pushHeartbeatIntervalID: ReturnType<typeof setInterval>

The let declaration is in the temporal dead zone until line 592 executes. While this works in practice because stop is only called during shutdown (after all module-level code runs), it's fragile — if the let is moved or the function is called earlier during initialization, it would throw a ReferenceError.

Recommendation: Move the declaration to before the selector registration:

let pushHeartbeatIntervalID: ReturnType<typeof setInterval> | undefined

sbp('sbp/selectors/register', {
  'backend/server/stop': async function () {
    clearInterval(pushHeartbeatIntervalID)
    // ...
  },
  // ...
})

// ... later in file ...
;(() => {
  pushHeartbeatIntervalID = setInterval(...)
})()

---

Issue 3: Unused parameter _contractID in buildSignedKvPayload

• Addressed
• Dismissed

⚪️ Low importance, high confidence

In routes-test-helpers.ts:23-24:

export function buildSignedKvPayload (
  _contractID: string,

The _contractID parameter is never used in the function body. Every call site passes owner.contractID as the first argument. Either the parameter should be used (e.g., included in the signed data for domain separation) or removed from both the signature and all call sites.

---

Issue 4: stopTestServer hangs if server was never started

• Addressed
• Dismissed

⚪️ Low importance, high confidence

In routes-test-helpers.ts:147-155:

export async function stopTestServer (): Promise<void> {
  await new Promise<void>((resolve) => {
    sbp('okTurtles.events/once', SERVER_EXITING, () => {
      sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
        resolve()
      })
    })
    sbp('okTurtles.events/emit', SERVER_EXITING)
  })
}

If stopTestServer is called without startTestServer having been called first, the SERVER_EXITING handler in index.ts (registered with once at module load) may fire, but the pubsub close handler depends on PUBSUB_INSTANCE being set. If it's not set, sbp('okTurtles.data/apply', PUBSUB_INSTANCE, ...) may throw or never resolve, causing the promise to hang.

Recommendation: Add a guard:

export async function stopTestServer (): Promise<void> {
  await new Promise<void>((resolve) => {
    sbp('okTurtles.events/once', SERVER_EXITING, () => {
      sbp('okTurtles.eventQueue/queueEvent', SERVER_EXITING, () => {
        resolve()
      })
    })
    sbp('okTurtles.events/emit', SERVER_EXITING)
  })
}

Or alternatively, ensure stopTestServer is only called in finally blocks after startTestServer succeeds (which the current tests do), making this a low-priority concern.

---

Issue 5: clearInterval on Bottleneck.Group private API is fragile

• Addressed
• Dismissed

⚪️ Low importance, medium confidence

In routes.ts:76-79:

clearInterval((limiterPerMinute as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerHour as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerDay as unknown as { interval: ReturnType<typeof setInterval> }).interval)

This accesses a private implementation detail of the Bottleneck library. If the library is updated and renames or removes the interval property, this code silently breaks (leaking timers). The comment acknowledges this, but no safeguard exists.

Recommendation: Wrap each clearInterval in a try-catch or check for the property:

for (const limiter of [limiterPerMinute, limiterPerHour, limiterPerDay] as unknown as { interval?: ReturnType<typeof setInterval> }[]) {
  if (limiter.interval != null) clearInterval(limiter.interval)
}

---

Issue 6: Test helper saltsAndEncryptedHashedPassword and decryptRegistrationRedemptionToken are exported but only used in tests

• Addressed
• Dismissed

⚪️ Low importance, high confidence

These functions in routes-test-helpers.ts:39-61 duplicate cryptographic logic from the application (likely @chelonia/lib internals). This is fine for a test helper file, but if the cryptographic protocol changes, these tests would need manual updates. Consider importing from the actual library if these helpers exist there.

---

Issue 7: Trailing comma inconsistency in build.d.ts

• Addressed
• Dismissed

⚪️ Low importance, high confidence

In src/types/build.d.ts:2-4:

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

The first two properties have trailing commas but the last does not. While TypeScript permits both, the project's StandardJS formatting (semiColons: false, singleQuote: true) doesn't enforce trailing commas. The original code had no trailing comma on the single property. The new code is internally consistent with itself but differs from the original style. Minor nit.

---

Non-issue observations

• The pingIntervalID type fix (setTimeout → setInterval) in pubsub.ts:186 is correct — the value is assigned from setInterval on line 251.
• The removeSignalHandlers + chelonia.persistentActions/unload additions in index.ts are properly sequenced.
• The worker path refactoring (import.meta.workerDir → separate ownerSizeTotalWorker/creditsWorker) is consistent across build.ts, server.ts, and build.d.ts.
• Test assertions are well-structured and match the actual route behaviors verified against routes.ts.

---

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

[corrideat]

/crush_fast

---

AI review started.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

Advanced AI Review

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

Click to expand review

Now I have enough context to write a thorough review. Let me compile my findings.## Code Review: Add Endpoint Tests

---

1. 🟡 Rate limiter cleanup relies on Bottleneck internal private API

• Addressed
• Dismissed

src/serve/routes.ts:78-80

clearInterval((limiterPerMinute as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerHour as unknown as { interval: ReturnType<typeof setInterval> }).interval)
clearInterval((limiterPerDay as unknown as { interval: ReturnType<typeof setInterval> }).interval)

This accesses an undocumented .interval property on Bottleneck.Group instances via as unknown as casting. If Bottleneck is ever upgraded and renames or removes this internal property, the clearInterval calls will silently do nothing (passing undefined to clearInterval is a no-op), causing the test process to hang due to lingering timers. The code comment acknowledges this is necessary, but consider adding a defensive check so the failure mode is loud rather than silent:

for (const limiter of [limiterPerMinute, limiterPerHour, limiterPerDay]) {
  const internal = limiter as unknown as { interval?: ReturnType<typeof setInterval> }
  if (internal.interval != null) {
    clearInterval(internal.interval)
  }
}

---

2. 🟡 Test server cannot restart after being stopped — running all test files together will hang

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:157-187

The stopTestServer function emits SERVER_EXITING, which consumes the module-level sbp('okTurtles.events/once', SERVER_EXITING, ...) handler registered in src/serve/index.ts:42. Because Deno caches modules, a second call to startTestServer() (from a different test file in the same process) will:

1. Call internal() → import('./index.ts') returns cached module
2. Call start() → import('./server.ts') returns cached module (the initialization IIFE at the bottom of server.ts doesn't re-run)
3. hapi.start() is never called again → SERVER_RUNNING is never emitted → startTestServer promise hangs forever

This means all four test files (routes-stateless.test.ts, routes-reads.test.ts, routes-kv.test.ts, routes-writes.test.ts, routes-zkpp.test.ts) cannot be run together in a single deno test invocation. They must each be run in separate processes, or the server lifecycle needs to support full restart.

---

3. 🟡 buildShelterAuthHeader doesn't match the signature format expected by verifyShelterAuthorizationHeader

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:99-104

export function buildShelterAuthHeader (contractID: string, SAK: ReturnType<typeof keygen>) {
  const nonceBytes = new Uint8Array(15)
  crypto.getRandomValues(nonceBytes)
  const data = `${contractID} ${Date.now()}.${Buffer.from(nonceBytes).toString('base64')}`
  return `shelter ${data}.${sign(SAK, data)}`
}

The resulting header format is: shelter <contractID> <timestamp>.<base64nonce>.<signature>. The signature is computed over <contractID> <timestamp>.<base64nonce>. This is used to construct a valid shelter auth header for tests. Since the tests that use this all pass (auth succeeds), the format appears correct for the current server implementation. However, this function duplicates crypto logic from @chelonia/lib/utils (verifyShelterAuthorizationHeader). If that implementation changes, the tests will break without a clear indication of why. Consider importing and using the actual client-side shelter auth builder from @chelonia/lib if one exists, or adding a comment documenting the expected format contract.

---

4. ⚪️ pushHeartbeatIntervalID declared but never initialized

• Addressed
• Dismissed

src/serve/server.ts:38

let pushHeartbeatIntervalID: ReturnType<typeof setInterval>

This variable is declared without initialization. It's only assigned later at server.ts:596 inside an IIFE. If backend/server/stop is called before the IIFE runs (e.g., during a failed startup), clearInterval(pushHeartbeatIntervalID) is called with an uninitialized variable. While clearInterval(undefined) is a safe no-op in Node.js/Deno, explicit initialization would make the intent clearer:

let pushHeartbeatIntervalID: ReturnType<typeof setInterval> | undefined

Or simply:

let pushHeartbeatIntervalID: ReturnType<typeof setInterval> | undefined = undefined

---

5. ⚪️ _contractID parameter unused in buildSignedKvPayload

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:27

export function buildSignedKvPayload (
  _contractID: string,
  key: string,
  height: number,
  data: unknown,
  SAK: ReturnType<typeof keygen>
)

The _contractID parameter is declared but never referenced in the function body. Every caller passes owner.contractID as the first argument. If the contract ID isn't needed in the signed payload, remove the parameter to avoid confusion. If it should be included (e.g., to bind the signature to a specific contract), add it to the signed data:

const additionalData = contractID + key + heightStr

---

6. ⚪️ Tests codify server returning 500 for client errors

• Addressed
• Dismissed

src/serve/routes-writes.test.ts:346-356 and src/serve/routes-writes.test.ts:484-496

// POST /event with non-JSON payload returns 500
if (res.status !== 500) throw new Error(`Expected 500 but got ${res.status}`)

// POST /file rejects duplicate upload
if (res.status !== 500) throw new Error(`Expected 500 for duplicate but got ${res.status}`)

These tests document that the server returns HTTP 500 (Internal Server Error) for what are client errors: malformed JSON input and duplicate resource uploads. These should ideally be 400 (Bad Request) and 409 (Conflict) respectively. The tests correctly document current behavior, but these represent server-side bugs that should be fixed separately.

---

Summary

The changes are well-structured overall. The key concerns are:

• Request perms at runtime to connect to net #1 (fragile private API usage) and Command to convert from one database backend to another #2 (test server cannot restart) are the most actionable items.
• Disable semi-colons for deno fmt #3-Use esm pinning to specify precise versions of dependencies #6 are lower-priority improvements to robustness and clarity.

---

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

[taoeffect]

Nice work!