Support for differently named contracts.

[corrideat]

Closes #160

AI disclosure: Used GLM-5.3 for planning and code generation.

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

I'll review this PR thoroughly. Let me start by examining the relevant source files.Let me check the auth middleware and a few remaining details.Now let me verify the tests actually pass. First checking the test setup:Deno isn't available in this environment, so I'll complete a thorough static review. Let me examine the remaining pieces:Let me check how contract manifests/sources get deployed (to assess the new size checks) and whether the dashboard uses sizeTotal history:Let me do a final sweep for leftovers and check a couple of remaining details:One critical thing to verify — whether real contract bundles fit under the new 500KB cap:# Code Review: Support for differently named contracts

I reviewed the diff against the full source (routes, workers, db-utils, auth, config, tests). I could not execute deno test (Deno isn't installed in this environment), so this is a static review; where relevant I note what I verified by reading.

---

1. 🟡 Size-cap checks run before the cheap signup.disabled check and the rate limiter, and the source-size loop has no early exit

• Addressed
• Dismissed

src/serve/routes.ts:372-413 — for every unauthenticated first message, the server now performs up to 3 DB reads (manifest + contract source + contractSlim source) before the rate limiter runs:

if (!credentials?.billableContractID && deserializedHEAD.isFirstMessage) {
  if (Buffer.byteLength(payload) > nconf.get('server:signup:maxFirstMessageBytes')) {

let contractSizeBytes = 0
for (const hash of contractSourceHashes) {
  const source = await sbp('chelonia.db/get', hash)
  if (typeof source !== 'string') throw new HTTPException(422, { message: 'Missing contract source' })
  contractSizeBytes += Buffer.byteLength(source)
}
if (contractSizeBytes > nconf.get('server:signup:maxContractSizeBytes')) {

if (nconf.get('server:signup:disabled')) {
  throw new HTTPException(403, { message: 'Registration disabled' })
}
// rate limit signups in production
if (!SIGNUP_LIMIT_DISABLED) {

Two problems:

• Requests rejected with 413/422/422 never consume a rate-limit token, so an attacker can trigger the manifest + source reads (sources can be large: /file accepts up to fileUploadMaxBytes, default 30 MiB, and the loop reads them fully into memory to compute the sum) at line rate with no backstop. The old code only did one manifest read pre-limiter, so this widens the surface.
• Even with signup.disabled = true, the DB reads still happen before the 403.

Suggested reordering (cheap checks first, and bail out of the sum as soon as the cap is exceeded):

if (!credentials?.billableContractID && deserializedHEAD.isFirstMessage) {
  if (nconf.get('server:signup:disabled')) {
    throw new HTTPException(403, { message: 'Registration disabled' })
  }
  // rate limit signups in production
  if (!SIGNUP_LIMIT_DISABLED) {
    try {
      const keyedIp = limiterKey(ip)
      await limiterPerMinute.key(keyedIp).schedule(() => Promise.resolve())
      await limiterPerHour.key(keyedIp).schedule(() => Promise.resolve())
      await limiterPerDay.key(keyedIp).schedule(() => Promise.resolve())
    } catch {
      console.warn('rate limit hit for IP:', ip)
      throw new HTTPException(429, { message: 'Rate limit exceeded' })
    }
  }
  if (Buffer.byteLength(payload) > nconf.get('server:signup:maxFirstMessageBytes')) {
    throw new HTTPException(413, { message: 'First message exceeds size limit' })
  }
  // ... manifest parsing ...
  const maxContractSizeBytes = nconf.get('server:signup:maxContractSizeBytes')
  let contractSizeBytes = 0
  for (const hash of contractSourceHashes) {
    const source = await sbp('chelonia.db/get', hash)
    if (typeof source !== 'string') throw new HTTPException(422, { message: 'Missing contract source' })
    contractSizeBytes += Buffer.byteLength(source)
    if (contractSizeBytes > maxContractSizeBytes) {
      throw new HTTPException(413, { message: 'Contract source exceeds size limit' })
    }
  }
}

(Moving the limiter first also means garbage requests consume the per-IP reservoir, which is desirable.)

---

2. 🟡 The caps only bound the first message; the 10 MiB free allowance is farmable per registration and nothing yet blocks over-quota writes

• Addressed
• Dismissed

src/serve/routes.ts:372 and src/serve/creditsWorker.ts:252 — the new guards apply exclusively to deserializedHEAD.isFirstMessage. Subsequent POST /event messages to an unattributed-created contract have no per-entity cap (only the route-wide 1 MiB bodyLimit) and are not rate-limited, and each accepted registration mints a fresh billable entity with its own freeAllowanceBytes:

updateCredits(billableEntity, 'charge', Math.max(0, size - freeAllowanceBytes)).catch((e) => {

Since the name check is gone, any manifest deployed to the server (any authenticated user can deploy one via POST /file, or /dev-file in dev) can be used to register ownerless entities, each earning 10 MiB of free storage. With the default signup limits that is ~500 MiB/day/IP of free storage, plus unbounded event writes until billing freezes exist.

The issue thread explicitly defers the freeze ("enforce that users whose billable contracts run out of credits cannot be written to"), so this is a known gap rather than a regression — but the PR description says "Closes #160" while the enforcement half of the final decision is absent. Please make sure the follow-up is tracked as its own issue and consider linking it from the README's new section (README.md:452-460), which currently reads as if the 10 MiB tier were enforced ("free up to that threshold, billed beyond it, frozen if balance goes negative" — the frozen part is not implemented anywhere in this repo yet).

---

3. 🟡 readFreeAllowanceBytes uses parseInt, which silently misparses valid stored values

• Addressed
• Dismissed

src/serve/creditsWorker.ts:209:

const allowance = parseInt(stored ?? '', 10)

server.ts:595 stores the value with String(...). String(1e21) is '1e+21', and parseInt('1e+21', 10) is 1 — i.e., a (zod-valid) allowance of 10²¹ bytes silently becomes 1 byte, charging users for essentially their entire storage. parseInt('10.5') also silently truncates to 10. Since the function otherwise carefully handles invalid input, use Number() plus a safe-integer check:

const readFreeAllowanceBytes = async (): Promise<number> => {
  const stored = await sbp('chelonia.db/get', '_private_freeAllowanceBytes', { bypassCache: true })
  const allowance = stored == null ? NaN : Number(stored)
  if (!Number.isSafeInteger(allowance) || allowance < 0) {
    console.warn(`[creditsWorker] Invalid free allowance '${stored}', using 0`)
    return 0
  }
  return allowance
}

---

4. 🟡 sizeTotal in the billing history now records the billable size, but the updateCredits JSDoc (and entry-field semantics) still say "total size"

• Addressed
• Dismissed

src/serve/creditsWorker.ts:252 passes the post-allowance size into updateCredits, which stores it as sizeTotal (src/serve/creditsWorker.ts:105) and averages it into the coarse aggregate (sizeTotal: Math.floor(periodSize / totalPeriodLength), src/serve/creditsWorker.ts:176). The doc comment is now wrong:

 * @param amount - For 'charge', this is the current total size (bytes). For 'credit', the amount to add.

and GranularChargeEntry.sizeTotal (src/serve/creditsWorker.ts:15) now means two different things depending on whether an entry predates this change — an entity storing 8 MiB with a 10 MiB allowance gets sizeTotal: 0 entries, so actual usage is no longer derivable from history at all. At minimum, fix the docs:

 * @param amount - For 'charge', the billable size (bytes): the entity's total
 * owned size minus its free allowance. For 'credit', the amount to add.

Ideally, also record the raw size so the allowance is auditable from history:

granularHistory.unshift({
  type: 'charge',
  date,
  sizeTotal: amount,          // billable size the charge is based on
  rawSizeTotal: rawSize,      // actual owned size, for auditing
  ...
})

(keeping sizeTotal as-is avoids breaking existing readers of old entries).

---

5. ⚪️ A present-but-malformed contractSlim is silently excluded from the size check

• Addressed
• Dismissed

src/serve/routes.ts:384:

if (typeof contractSlim?.hash === 'string') contractSourceHashes.push(contractSlim.hash)

A manifest body with "contractSlim": {} (or a non-string hash) skips the slim source entirely instead of being rejected, so the branch tolerates shapes that handleEntry will later refuse. Treat a present-but-invalid contractSlim like a missing contract.hash:

if (typeof contract?.hash !== 'string') throw new Error('missing contract hash')
contractSourceHashes = [contract.hash]
if (contractSlim != null) {
  if (typeof contractSlim.hash !== 'string') throw new Error('missing contractSlim hash')
  contractSourceHashes.push(contractSlim.hash)
}

---

6. ⚪️ Comparisons against nconf.get() can be silently bypassed by non-numeric env overrides

• Addressed
• Dismissed

src/serve/routes.ts:373 and src/serve/routes.ts:395:

if (Buffer.byteLength(payload) > nconf.get('server:signup:maxFirstMessageBytes')) {

nconf.env({ parseValues: true }) leaves values that fail JSON.parse as strings, so SERVER__SIGNUP__MAXFIRSTMESSAGEBYTES=abc makes the comparison number > 'abc' → NaN > ... → false, disabling the cap with no error (the TOML validator only checks the file, not env/CLI). The pre-existing ||-fallback reads share this hazard, but the new caps are security-relevant, so coerce explicitly:

const maxFirstMessageBytes = Number(nconf.get('server:signup:maxFirstMessageBytes'))
if (!(maxFirstMessageBytes > 0) || Buffer.byteLength(payload) > maxFirstMessageBytes) {
  throw new HTTPException(413, { message: 'First message exceeds size limit' })
}

---

7. ⚪️ billing-config.test.ts is missing the import 'jsr:@db/sqlite' workaround used by every sibling test that starts the server

• Addressed
• Dismissed

Every other test file in src/serve/ that starts the server/DB begins with import 'jsr:@db/sqlite' — src/serve/server-id.test.ts:10-12 documents it as "loaded purely to keep the Deno memory-leak checker happy". The new src/serve/billing-config.test.ts:1-2 starts the full server (workers included) without it:

import { assertEquals } from 'jsr:@std/assert'
import { sbp, startTestServer, stopTestServer } from './routes-test-helpers.ts'

Add the same side-effect import for consistency and to avoid the sanitizer flakiness the others guard against.

---

8. ⚪️ The new ownerSizeTotalWorker test shadows the module-level worker

• Addressed
• Dismissed

src/serve/ownerSizeTotalWorker.test.ts:287 declares const worker while the file already has a module-level let worker (:7) that the updateSize helper closes over (:17-21):

const worker = createWorker(new URL('./ownerSizeTotalWorker.ts', import.meta.url).toString())

By the time this test runs, the module-level worker has been terminated by the first test's finally, so any accidental use of the updateSize() helper inside this test would RPC a dead worker. Rename the local:

const sizeWorker = createWorker(new URL('./ownerSizeTotalWorker.ts', import.meta.url).toString())
await sizeWorker.ready

(and update the references in the test body).

---

9. ⚪️ Namespace registration is now silently ignored for attributed first messages

• Addressed
• Dismissed

src/serve/routes.ts:443-458 — the old code registered a name whenever the processed contract's type was gi.contracts/identity, which included attributed identity contracts; the new code silently no-ops when credentials are present, and the test codifies "the name registration was silently ignored". A client that mistakenly sends both an authorization header and shelter-namespace-registration gets a 200 and silently loses the username. Consider rejecting the combination instead of ignoring it, so misbehaving clients surface:

const name = validatedHeaders['shelter-namespace-registration']
if (name) {
  if (credentials?.billableContractID) {
    throw new HTTPException(400, { message: 'Name registration requires an unattributed first message' })
  }
  ...
}

(If the silent skip is intentional for compatibility, a comment saying so at src/serve/routes.ts:440-443 would help.)

---

10. ⚪️ DRY: duplicated randCID, ad-hoc manifest parsing vs. the existing zod schema, and a duplicated !credentials?.billableContractID condition

• Addressed

• Dismissed

• randCID is defined identically in src/serve/creditsWorker.test.ts:23-27 and src/serve/ownerSizeTotalWorker.test.ts:11-15. Move it to routes-test-helpers.ts (which already exports shared test utilities) and import it in both.

• The manifest-body parsing at src/serve/routes.ts:381-384 re-implements, untyped, what src/deploy.ts:13-16 already validates with zod:

  const ContractBodySchema = z.object({
    contract: z.object({ file: z.string() }),
    contractSlim: z.object({ file: z.string() }).optional()
  })

  A shared schema including hash (e.g. in src/serve/backend-schemas.ts or src/utils.ts) would give both call sites typed, consistent validation:

  export const ContractBodySchema = z.object({
    contract: z.object({ hash: z.string(), file: z.string() }),
    contractSlim: z.object({ hash: z.string(), file: z.string() }).optional()
  })

• !credentials?.billableContractID is evaluated twice in the same handler (src/serve/routes.ts:372 and :443); the name-registration block can live inside the existing else branch, which also makes the "only new billable entities register names" rule structural rather than re-derived:

  if (credentials?.billableContractID) {
    await sbp('backend/server/saveOwner', credentials.billableContractID, deserializedHEAD.contractID)
  } else {
    await sbp('backend/server/registerBillableEntity', deserializedHEAD.contractID)
    const name = validatedHeaders['shelter-namespace-registration']
    if (name) {
      // ...register name...
    }
  }

---

11. ⚪️ "Separate worker process" wording is inaccurate

• Addressed
• Dismissed

src/config-defaults.ts:26-27, README.md:458, src/serve/billing-config.test.ts:5, and src/serve/creditsWorker.ts:202-203 all say the credits worker "runs in a separate process". Deno Workers are threads/isolates within the server process (see src/serve/createWorker.ts:35 — new Worker(...), same process, terminated by worker.terminate()); the actual reason for persisting the allowance is that each worker has its own module instance and therefore no access to the main thread's nconf state. Suggested wording, e.g. in config-defaults.ts:

// Billing settings. The credits worker runs in a Worker thread with its own
// module state (no access to nconf), so `freeAllowanceBytes` is persisted to
// the database at startup ...

---

12. ⚪️ The default maxContractSizeBytes has little headroom over the real identity bundle, and no test exercises registration with a realistic manifest

• Addressed
• Dismissed

src/config-defaults.ts:18 sets maxContractSizeBytes: 500 * 1024 (512 000), and src/serve/routes.ts:389-394 counts contract + contractSlim combined. The actual Group Income identity bundle in this repo is 306 009 + 7 574 = 313 583 bytes (test/assets/identity.js, test/assets/identity-slim.js) — about 61% of the cap. All new tests (src/serve/routes-signup.test.ts) use tiny fake sources, so if the identity bundle grows past ~504 KB combined, every production signup starts failing with a 413 that operators won't easily connect to contract size. Consider either a test that registers using test/assets/identity.js (locks the default to reality) or a note in README.md:449-451 that the cap counts full + slim combined and must stay above the deployed identity bundle.

---

Summary: no outright correctness bugs found in the happy paths — the free-allowance anchoring logic in creditsWorker.ts is sound (zero-charge entries correctly anchor later charges without retroactive billing, and startTime prevents charging downtime), the worker/DB wiring matches existing patterns, and the config/schema/test-defaults changes are mutually consistent (5120 / 512000 / 10485760 everywhere). The highest-value fixes are the request-ordering hardening in issue 1 and explicitly tracking the deferred enforcement from issue 2.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[corrideat]

These added as per @taoeffect's request. It doesn't seem useful to me to check for maxContractSizeBytes, as the contract source size is not a concern here (contract sources are uploaded by a different mechanism).

[akhileshthite]

Ordering question. The manifest read and these source reads now happen before
the signup.disabled check (line 398) and before the rate limiter below it.

So on a server with signup.disabled = true, every request still does one DB
read for the manifest plus up to maxContractSizeBytes of source reads, 500 KB
by default, before it returns 403. Same for an IP that is already over the rate
limit, it pays that cost on every attempt.

Before this PR the only work ahead of those two checks was reading the manifest
and pulling the name out of it? Now it is bounded by config, but the bound is
500 KB per request.

Can signup.disabled and the rate limiter move above the manifest read? The
maxFirstMessageBytes check at line 373 can stay where it is, the payload is
already in memory so it costs nothing.

[corrideat]

Good point. I think we could move those pre-existing checks to happen before these (like in my other comment, I also think we don't need the contract source checks)

[corrideat]

/review

---

AI review started.

[github-actions]

Advanced AI Review

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

Click to expand review

Deno isn't available here, so I'll do a thorough static review. Let me examine the remaining relevant files.The nconf semantics matter for the test helpers. Let me check nconf's actual source online.# Review: Support for differently named contracts

I reviewed the full diff (5be6a25..68121e2) against the surrounding codebase (routes.ts, server.ts, database.ts, db-utils.ts, ownerSizeTotalWorker.ts, nconf semantics, the dashboard, and the other route test files). No 🔴-severity code bugs found; the check ordering, the billing arithmetic, and the size-cap logic are all sound. The issues below are in descending order of importance.

---

1. 🟡 README claims the signup limits bound unpaid storage per client — they don't

• Addressed
• Dismissed

README.md:487-490:

Note that charges are currently recorded but not enforced: a billable entity
whose balance goes negative keeps being served. Since each accepted
registration creates an entity with its own free allowance, the signup limits
above are what bounds how much unpaid storage a single client can accumulate.

The final sentence is contradicted by the sentence right before it. Only the first message of a contract is size-capped (src/serve/routes.ts:385-433) and rate-limited (src/serve/routes.ts:392-403); subsequent messages to the same contract are bounded only by the 1 MiB bodyLimit (src/serve/routes.ts:354) with no rate limit at all. Since negative balances keep being served, a single registration can accumulate unbounded unpaid storage. What the signup limits actually bound is the number of free allowances a client can claim.

Suggested rewording:

Note that charges are currently recorded but not enforced: a billable entity
whose balance goes negative keeps being served. Until enforcement is added,
the signup limits bound only how many free allowances a client can claim —
not how much unpaid storage a single registration can accumulate, since
messages other than the first are neither size-capped nor rate-limited.

(Minor grammar note in the same sentence: "are what bounds" → "are what bound", though rewording as above makes it moot.)

---

2. 🟡 readIntConfig accepts '' and booleans, silently disabling limits/tiers without a warning

• Addressed
• Dismissed

src/serve/config-utils.ts:26-33:

  const raw = nconf.get(key)
  if (raw == null) return fallback
  const value = typeof raw === 'number' ? raw : Number(raw)
  if (!Number.isSafeInteger(value) || value < 0 || (value === 0 && !allowZero)) {

Two coercions slip through the guard that this file exists to provide:

• Number('') === 0, so an empty env var (server__billing__freeAllowanceBytes= chel serve — lowercase env names do reach nconf keys) is accepted by nonNegativeIntConfig as 0, silently disabling the free tier for the whole server and charging everyone from the first byte — with no warning, because 0 is "valid".
• Number(true) === 1, so server__signup__maxFirstMessageBytes=true (parsed to a boolean by nconf's parseValues) sets the message cap to 1 byte, again without a warning.

Fix by only coercing non-empty strings:

  const raw = nconf.get(key)
  if (raw == null) return fallback
  const value = typeof raw === 'number'
    ? raw
    : (typeof raw === 'string' && raw.trim() !== '' ? Number(raw) : NaN)
  if (!Number.isSafeInteger(value) || value < 0 || (value === 0 && !allowZero)) {
    console.warn(`[config] Invalid value for '${key}': ${JSON.stringify(raw)}. Using ${fallback}.`)
    return fallback
  }

---

3. 🟡 DRY: the new default values are re-hardcoded in five test locations instead of imported

• Addressed
• Dismissed

The literals 5 * 1024, 500 * 1024 and 10 * 1024 * 1024 now exist in src/config-defaults.ts:17-18,34 (the single source of truth, explicitly documented as such in src/init.ts:10-12) and in:

• src/serve/routes-test-helpers.ts:270-276
• src/serve/routes-signup.test.ts:63,81,101,112
• src/serve/billing-config.test.ts:24 (comment even acknowledges "matching the test-server default in routes-test-helpers.ts")
• src/serve/config-utils.test.ts:14-15
• src/serve/archive-mode.test.ts:19

If a default changes, these tests keep asserting the old value and give false confidence. config-defaults.ts is dependency-free, so it can be imported anywhere safely:

// src/serve/routes-test-helpers.ts
import { nconfDefaults } from '../config-defaults.ts'
// ...
        signup: {
          disabled: false,
          maxFirstMessageBytes: nconfDefaults.server.signup.maxFirstMessageBytes,
          maxContractSizeBytes: nconfDefaults.server.signup.maxContractSizeBytes,
          limit: { disabled: false, minute: 100, hour: 1000, day: 10000 }
        },
        billing: {
          freeAllowanceBytes: nconfDefaults.server.billing.freeAllowanceBytes
        },

and in the tests:

// src/serve/billing-config.test.ts
import { nconfDefaults } from '../config-defaults.ts'
// ...
        assertEquals(stored, String(nconfDefaults.server.billing.freeAllowanceBytes))

(config-utils.test.ts can do the same for DEFAULT_MAX_FIRST_MESSAGE_BYTES / DEFAULT_FREE_ALLOWANCE_BYTES.)

---

4. ⚪️ contract.hash / contractSlim.hash are used as raw DB keys without CID validation

• Addressed
• Dismissed

src/serve/routes.ts:410-418:

              if (typeof contract?.hash !== 'string') throw new Error('missing contract hash')
              contractSourceHashes = [contract.hash]
              // A present-but-malformed `contractSlim` is rejected rather than
              // skipped: silently ignoring it would leave its source out of the
              // size check while `handleEntry` would refuse the manifest anyway
              if (contractSlim != null) {
                if (typeof contractSlim.hash !== 'string') throw new Error('missing contractSlim hash')

These attacker-influenced strings (the manifest body is authored by whoever deployed the manifest; in dev /dev-file lets anyone deploy one) are passed straight to chelonia.db/get at src/serve/routes.ts:427, so they can target arbitrary keys (e.g. _private_*). Exposure today is only a length-based accept/reject oracle plus a bounded DB read, and handleEntry rejects the message later anyway — but validating the multicode is cheap defense-in-depth and matches the CID check already done for the manifest at src/serve/routes.ts:371-373:

              if (maybeParseCID(contract?.hash ?? '')?.code !== multicodes.SHELTER_CONTRACT_TEXT) {
                throw new Error('invalid contract hash')
              }
              contractSourceHashes = [contract.hash]
              if (contractSlim != null) {
                if (maybeParseCID(contractSlim.hash ?? '')?.code !== multicodes.SHELTER_CONTRACT_TEXT) {
                  throw new Error('invalid contractSlim hash')
                }
                contractSourceHashes.push(contractSlim.hash)
              }

---

5. ⚪️ maxFirstMessageBytes above 1 MiB is silently unreachable, and the rate limits only apply in production — neither is documented

• Addressed
• Dismissed

src/serve/routes.ts:311 reads the cap from config, but src/serve/routes.ts:354:

    bodyLimit({ maxSize: MEGABYTE }),

rejects anything over 1 MiB first (with its own 413) before the handler runs, so an operator who raises maxFirstMessageBytes beyond 1048576 gets no effect and no warning. Relatedly, README.md:461 documents the per-IP limits ("reject with 429 when an IP exceeds a limit") without noting that SIGNUP_LIMIT_DISABLED = process.env.NODE_ENV !== 'production' || ... (src/serve/routes.ts:307) disables them entirely outside production, which will confuse anyone testing on a dev server.

Cheapest fix is documentation; a schema bound would also work:

      maxFirstMessageBytes: z.optional(positiveInt.max(1048576, 'cannot exceed the 1 MiB /event body limit')),

---

6. ⚪️ sizeTotal is now a misnomer — it stores the billable size

• Addressed
• Dismissed

src/serve/creditsWorker.ts:15-18 documents the change, and src/serve/creditsWorker.ts:111 writes it:

  // The size the charge was computed from, i.e. the billable size: total owned
  // size minus the free allowance. Entries written before the free allowance
  // existed hold the total owned size instead, since the two were the same.
  sizeTotal: number;

The coarse aggregation (src/serve/creditsWorker.ts:167,182) then averages this field into _private_ownerBalanceHistoryCoarse_* entries, so (a) the coarse sizeTotal now means "average billable size" with no comment saying so, and (b) averages computed across the upgrade boundary mix total-size and billable-size entries into one meaningless number. Nothing in this repo consumes these keys yet, so this is a naming/semantics footgun for future consumers rather than a live bug. If the serialized field name can't change (old entries must stay readable), at minimum mirror the comment onto CoarseHistoryEntry.sizeTotal (src/serve/creditsWorker.ts:33); alternatively write a billableSize field on new entries and read entry.billableSize ?? entry.sizeTotal in the reducer.

---

7. ⚪️ PICOCREDITS_PER_BYTESECOND is duplicated in the test instead of shared

• Addressed
• Dismissed

src/serve/creditsWorker.test.ts:7-8:

// Mirrors `PICOCREDITS_PER_BYTESECOND` in creditsWorker.ts, which is not exported
const PICOCREDITS_PER_BYTESECOND = 10n

Simply exporting it from creditsWorker.ts and importing it here is not an option (importing that module in the main test process would register its chelonia.db/* RPC selectors and shadow the real database selectors, as billing-config.test.ts:5-8 notes). The right home is the side-effect-free constants module both files already import:

// src/serve/constants.ts
export const PICOCREDITS_PER_BYTESECOND = 10n

// src/serve/creditsWorker.ts
import { CREDITS_WORKER_TASK_TIME_INTERVAL as TASK_TIME_INTERVAL, PICOCREDITS_PER_BYTESECOND } from './constants.ts'

// src/serve/creditsWorker.test.ts
import { PICOCREDITS_PER_BYTESECOND } from './constants.ts'

---

8. ⚪️ CONFIG_KEYS in the archive-mode test can silently drift from applyConfig

• Addressed
• Dismissed

src/serve/archive-mode.test.ts:24-35 maintains the cleanup list by hand:

const CONFIG_KEYS = [
  'server_id',
  'server:host',
  ...
]

while the keys are set individually in applyConfig (src/serve/archive-mode.test.ts:37-50). If a key is later added to applyConfig but not to CONFIG_KEYS, it leaks into nconf's process-global memory store for the rest of the test run. Deriving both from one object removes the drift:

const applyConfig = (dirname: string, archiveMode: boolean): string[] => {
  nconf.use('memory')
  const config: Record<string, unknown> = {
    server_id: 'archive-mode-test-id',
    'server:host': '127.0.0.1',
    'server:port': 0,
    'server:appDir': '.',
    'server:archiveMode': archiveMode,
    'server:billing:freeAllowanceBytes': FREE_ALLOWANCE_BYTES,
    'server:messages': [],
    'database:backend': 'fs',
    'database:backendOptions': { fs: { dirname } },
    'database:lruNumItems': 100
  }
  for (const [key, value] of Object.entries(config)) nconf.set(key, value)
  return Object.keys(config)
}

and in the finally block, clear the keys returned by the last applyConfig call.

---

Summary: the core change (name-agnostic registration guarded by size caps, per-entity free allowance in the credits worker) is implemented and documented coherently, the check ordering strictly improves on the old code (DB reads now happen after rate limiting), and the test coverage is thorough. The actionable items are the two documentation accuracy fixes (#1, #5), the config-coercion hole (#2), and the DRY cleanups (#3, #7).

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[corrideat]

LGTM. README.md issues can be addressed in #164

[akhileshthite]

Looks ready, thanks!

[taoeffect]

Great work @corrideat! Need a docs update to merge!

[akhileshthite]

Tested this against our TodoMVC. Renamed our contract from
gi.contracts/identity to todomvc/identity, dropped the workaround, and ran
the suite against a local build of this branch: all 35 e2e tests pass,
including signup, username registration and login on a browser that has never
seen the account.

The same rename against the published 3.4.0 fails every signup with a 401, so
the tests really are exercising this change and not just passing anyway.

[taoeffect]

@corrideat Review by Opus 5.5 ready!

Details

Code Review

Base: master
Head: 160-support-differently-named-identity-contracts
Date: 2026-09-25
Model: anthropic/claude-opus-5-5

---

Context: #160. The gi.contracts/identity name check is gone. Ownerless first messages are now gated by a 5 KiB size cap (routes.ts:312-326). The kill switch and per-IP limits are unchanged. Every billable entity gets freeAllowanceBytes of uncharged storage (creditsWorker.ts:279). The billing arithmetic is correct: bytes on both sides, only the excess is charged, and the charge uses the current size times the elapsed time. The findings below are about how well the limits hold up, dead code, and test quality.

1. 🟡 Per-IP signup limits reset after ~5 idle minutes, so the hourly and daily caps are not enforced

• Addressed
• Dismissed

src/serve/signup-rate-limit.ts:89-97 builds each Bottleneck.Group without timeout, so the Group default of 300 000 ms applies (node_modules/bottleneck/lib/Group.js:190). Every timeout / 2, auto-cleanup deletes any per-key limiter where _nextRequest + timeout < now (LocalDatastore.js:137). _nextRequest only moves forward on a successful registration (LocalDatastore.js:222). Requests rejected by the LEAK strategy never touch it.

So once an IP exhausts its hourly quota, its perHour and perDay limiters are deleted 5–7.5 minutes after its last success, and they come back with a full reservoir. With the defaults (2/10/50), one IP or /64 can register about 10 accounts every ~12 minutes. That is roughly 48/hour and more than 1,000/day, and the daily limit never triggers.

The limiter config is the same as on master. This branch makes it load-bearing, though:

• creditsWorker.ts:275-276 states that "the per-IP signup rate limits bound how many [free allowances] can be created".
• docs/signup-and-billing.md:19-22 documents 10/hour and 50/day.

Each registration is now worth 10 MiB of free storage, and any contract name qualifies.

const group = (reservoir: number, intervalMs: number): Bottleneck.Group => {
  return new Bottleneck.Group({
    strategy: Bottleneck.strategy.LEAK,
    highWater: 0,
    reservoir,
    reservoirRefreshInterval: intervalMs,
    reservoirRefreshAmount: reservoir,
    // Keep a key alive for its whole window; the default (5 min) silently
    // resets the hourly/daily buckets
    timeout: intervalMs
  })
}

Trade-off: the 250 ms per-key heartbeats then live for up to a day per distinct IP. A plain Map<key, { count, windowStart }> fixed-window counter would avoid both the timers and the Bottleneck private-field hacks in disposeSignupLimiters. Add a fake-clock test for this, because none of the current tests advance time.

2. 🟡 The default maxFirstMessageBytes leaves ~15% headroom for real Group Income registrations, and the "realistic" fixture hides it

• Addressed
• Dismissed

I measured the 54 identity first messages in a local Group Income dev database. The current client's registrations are 4,344 bytes (22 of them) and older ones are 3,661 bytes (32). The default cap is 5,120 (src/config-defaults.ts:17). The current client is therefore at 85% of the cap, 776 bytes away from a 413 on every signup.

src/serve/routes-signup.test.ts:61-82 is meant to catch exactly this ("if clients grow their key set enough to cross the cap… this fails first", with a 75% threshold at line 73). It passes because realisticKeys (src/serve/routes-test-helpers.ts:214-247) builds only 4 keys (#csk, #cek, #pek, #sak). gi.actions/identity/create sends 7 (ipk, iek, csk, cek, pek, dmk, #sak), with explicit permissions and allowedActions.

Fix both sides:

• Build the fixture from the real key set, or check in a recorded first message under test/assets/.
• Raise the default, e.g. maxFirstMessageBytes: 8 * 1024. The maintainers only proposed 5 KB as "some reasonable value, like 5kb", so a higher default fits the issue.

3. 🟡 signup-guard.ts is dead code, but it ships with a test suite, a stale header, and a commented-out snippet in /file that uses an undefined constant

• Addressed
• Dismissed

After commit 39c1c5c nothing imports src/serve/signup-guard.ts except its own test. Line 1 now says "not currently in use", but the stale references remain:

• Lines 2-7 still describe it as the POST /event registration guard and send readers to docs/signup-and-billing.md "for what these caps are for". That doc no longer mentions a contract-source cap.
• The src/serve/signup-guard.test.ts:1-6 header says "Tests for the ownerless-first-message guards… These used to be inline in the POST /event handler". The suite name ("contract source size cap") still implies registration enforces such a cap.
• AGENTS.md:58 lists it as "Size/manifest checks for ownerless first messages".
• src/serve/routes.ts:598-610 puts a commented-out block at the top of the /file handler. It references deserializedHEAD, which only exists in /event, and SIGNUP_MAX_CONTRACT_SIZE_BYTES, which is defined nowhere.

Keeping untested-in-production helpers alive through a test suite costs maintenance for no benefit, and git history already preserves them. Delete signup-guard.ts, signup-guard.test.ts, the /file comment block and the AGENTS.md line. If a future contract-upload cap needs the idea, one line in docs/signup-and-billing.md pointing at commit f45330b is enough.

4. 🟡 createTestContractRegistration keeps dead options from the removed contract-source checks, and still defaults to the special name

• Addressed
• Dismissed

src/serve/routes-test-helpers.ts:119-166 still takes sourceBytes, slimSourceBytes, malformedContractSlim and contractHashOverride, with comments like "a manifest whose slim source could not be size-checked" and "for tests that check what a manifest is allowed to point at". The only caller, routes-signup.test.ts, passes just name, keys and messagePaddingBytes. The default name = 'gi.contracts/identity' (line 125) is never used, and it brings back the special-cased name this branch removes.

export async function createTestContractRegistration ({
  name,
  messagePaddingBytes = 0,
  keys = 'minimal'
}: {
  name: string
  messagePaddingBytes?: number
  keys?: 'minimal' | 'realistic'
}): Promise<{ serialized: string, contractID: string }> {
  const manifestSigningKey = keygen(EDWARDS25519SHA512BATCH)
  const contractSource = `export default {} // ${name}`
  // ...drop paddedSource, contractSlim branches and contractHashOverride

5. 🟡 The identity+DM test passes ultimateOwnerID explicitly, so the ownership chain it claims to test never runs

• Addressed
• Dismissed

The comment at src/serve/ownerSizeTotalWorker.test.ts:273-277 says a DM counts toward the identity through its _private_owner_ chain. But the DM updates at lines 306-315 pass ultimateOwnerID: identityID. The worker caches that value (ownerSizeTotalWorker.ts:143-145), and computeSizeTaskDeltas uses the cache instead of calling lookupUltimateOwner (line 173). As a result:

• The _private_owner_ and _private_resources_ writes (lines 302-303) have no effect on the outcome.
• The test would pass even if the owner chain were broken.
• Production DM messages and KV writes never supply ultimateOwnerID, so the lookup path the free allowance depends on is the one left untested.

ownerSizeTotalWorker.ts is not changed on this branch either. Let the lookup run:

      await sizeWorker.rpcSbp('worker/updateSizeSideEffects', { resourceID: dmID, size: 600 })
      await sizeWorker.rpcSbp('worker/updateSizeSideEffects', { resourceID: dmID, size: 200 })

The _private_billable_entities append for the identity (line 289) is also irrelevant to this worker.

6. ⚪ Unrecognized values for server:signup:disabled and server:archiveMode now fail open

• Addressed
• Dismissed

booleanConfig (src/serve/config-utils.ts:107-120) warns and returns the default (false) for spellings it doesn't recognize. On master, any truthy value turned these on. So server__signup__disabled=Y, =disable or =2 now leaves registration open (routes.ts:317), and server__archiveMode=readonly makes an archive server writable (server.ts:427, database.ts, routes.ts). Falling back to the default is right for limit:disabled, where false is the safe side. For these two keys it is the unsafe side.

export const booleanConfig = (key: string, onInvalid?: boolean): boolean => {
  const fallback = requireDefault<boolean>(key, 'boolean')
  // ...
  const safe = onInvalid ?? fallback
  warn(key, raw, 'not a boolean', safe)
  return safe
}
// routes.ts
if (booleanConfig('server:signup:disabled', true)) { ... }

7. ⚪ Negative balances have no effect, and the docs don't say the freeze from #160 is deferred

• Addressed
• Dismissed

In #160 the maintainers decided to charge usage above the allowance "and freeze their account if credit balance goes below 0". creditsWorker.ts only writes _private_ownerPicocreditBalance_*. Nothing reads that key, and nothing adds credits, so every entity over 10 MiB drifts negative with no consequence. The issue lists enforcement under "Later on", but docs/signup-and-billing.md:32-34 only says the worker "debits the entity's balance", which reads as if it were enforced.

Balances are recorded but not yet enforced: an entity whose balance drops
below zero is not frozen and can keep writing (see #160).

8. ⚪ disposeSignupLimiters leaves every per-IP limiter's heartbeat running

• Addressed
• Dismissed

Each group.key(ip) creates a limiter whose datastore starts a 250 ms setInterval heartbeat (LocalDatastore.js:29-54; it is unref'd, so it doesn't block exit). Group.disconnect() only closes Redis connections, and clearing g.interval (src/serve/signup-rate-limit.ts:131-133) stops the only thing that would ever call deleteKey. After registerRoutes re-runs or stopRateLimiters is called, the old per-IP limiters keep ticking and keep their Groups alive. The pattern is inherited from master, but the new module is the natural place to fix it:

  for (const g of groups) {
    clearInterval((g as unknown as { interval: ReturnType<typeof setInterval> }).interval)
  }
  await Promise.allSettled(groups.flatMap(g => g.keys().map(k => g.deleteKey(k))))

This goes away if #1 is solved with a Map-based counter.

9. ⚪ Stale "registration section in README.md" reference, and name registration for any ownerless contract is undocumented

• Addressed
• Dismissed

src/serve/routes.ts:354-355 sends readers to "the registration section in README.md", which doesn't exist; commit 1667fd8 moved that text to docs/signup-and-billing.md. Neither document mentions a behavior change clients will see: any ownerless root contract can now claim a name through shelter-namespace-registration, and attributed first messages silently ignore the header. Point the comment at docs/signup-and-billing.md and add a short paragraph on name registration there.

10. ⚪ "Bounds how much data can be written 'for free'" misdescribes the first-message cap

• Addressed
• Dismissed

This wording appears in src/config-defaults.ts:14-16, src/init.ts:46-47 and docs/signup-and-billing.md:14-16. The same branch gives every registration 10 MiB of free storage, so the free amount is freeAllowanceBytes, not 5120. What the cap actually bounds is the single unauthenticated write that creates the entity.

      // Size sanity cap for unattributed (ownerless) first messages, i.e.
      // identity contract registration: the one write an unauthenticated
      // client can make. See POST /event in src/serve/routes.ts.

11. ⚪ billing-config.test.ts is a weaker duplicate of archive-mode.test.ts

• Addressed
• Dismissed

src/serve/billing-config.test.ts:17-21 compares _private_freeAllowanceBytes to nconfDefaults.server.billing.freeAllowanceBytes. That is also the value startTestServer injects and the value nonNegativeIntConfig falls back to, so the test still passes if server.ts ignores configuration entirely. archive-mode.test.ts:46-58 already covers the same startServer path with a non-default allowance. Delete billing-config.test.ts.

12. ⚪ test/readme-config.test.ts pins documentation prose, and its "1 MiB" is hard-coded

• Addressed
• Dismissed

Lines 34-54 match exact phrases such as (defaults `2`/`10`/`50` per IP) and `NODE_ENV=production`, so rewording the docs fails the build even when every number is right. Line 53 is also internally wrong:

assertDocumented(doc, 'the POST /event request body limit', `over 1 MiB (${MAX_EVENT_BODY_BYTES} bytes)`)

If the constant changed, the test would require the doc to say "over 1 MiB (2097152 bytes)". The chel init template is already derived from nconfDefaults and pinned by init.test.ts. Either drop this test or assert only the interpolated numbers (e.g. `(${MAX_EVENT_BODY_BYTES} bytes)`).

13. ⚪ Test steps that don't check what their names claim

• Addressed

• Dismissed

• src/validateConfig.test.ts:149-153 ("errors on negative or fractional byte-size settings") only tries -1 and -1024. Add 1.5 and 1024.5 so that dropping .int() would be caught. The step at 167-177 (typo suggestion inside server.billing) runs the same generic code as the existing typo steps, and the knownKeysFor step already proves server.billing is visited. Drop it.

• src/serve/signup-rate-limit.test.ts:76-83 uses { minute: 2, hour: 3, day: 4 }, so every rejection comes from the minute bucket. The test would still pass if perHour or perDay were never consulted. Add cases where a longer window runs out first (e.g. { minute: 10, hour: 2, day: 10 } and { minute: 10, hour: 10, day: 2 }).

• The step "a missing or invalid allowance falls back to charging the full size" in src/serve/creditsWorker.test.ts:98-105 never deletes _private_freeAllowanceBytes, so the stored == null branch is never run.

• src/serve/config-utils.test.ts:110-118 ("every rejected value is reported") doesn't reset the per-key dedupe state first. It passes only because the previous step ended on [5]. Start it with withValue(KEY, 1234, () => positiveIntConfig(KEY)), as the "repeated configuration warnings" test does.

14. ⚪ Merge resolution re-expanded the AGENTS.md tree that master had condensed, and the new listing is already wrong

• Addressed
• Dismissed

Master (#162) replaced the per-file scripts/ and test/ listings with one-liners. Merge f635349 restored the per-file children (AGENTS.md:67-83). The test/ list omits compile-smoke.test.ts, eventsAfter.test.ts, utils.test.ts and test-helpers.ts. The serve/ list names the dead signup-guard.ts (#3) and leaves out the billing workers. Keep master's condensed form and mention the new areas in the serve/ one-liner:

└── serve/               # Server implementation (routes, DB backends, pubsub, dashboard, signup limits, billing workers)

15. ⚪ reclaimForeignSubscriptions is still read with !!nconf.get, three lines above the new booleanConfig call

• Addressed
• Dismissed

src/serve/server.ts:424 has the exact bug booleanConfig exists to prevent: server__reclaimForeignSubscriptions=off turns on the destructive deletion of foreign push subscriptions. The bug predates this branch, but the fix is one line and nconfDefaults already has a boolean default:

  const reclaimForeignSubscriptions = booleanConfig('server:reclaimForeignSubscriptions')

16. ⚪ Uncompressed IPv6 addresses with an IPv4 tail are rejected with a 429

• Addressed
• Dismissed

limiterKey (src/serve/signup-rate-limit.ts:31-87, moved from master) counts a dotted IPv4 tail as one segment. So 0:0:0:0:0:ffff:203.0.113.7, which isIP accepts, has 7 segments, reaches throw new Error('Invalid IPv6 address'), and consumeSignupToken turns that into a permanent 429. This only happens if a proxy writes X-Real-IP in that form, so confidence is low.

    const v4Tail = address.slice(address.lastIndexOf(':') + 1)
    if (isIP(v4Tail) === 4) return v4Tail

17. ⚪ Minor cleanups

• Addressed

• Dismissed

• Duplicate 1 MiB constant: src/serve/constants.ts:9 adds MAX_EVENT_BODY_BYTES = 1048576 while routes.ts:37 keeps const MEGABYTE = 1048576. Export MEGABYTE from constants.ts and set MAX_EVENT_BODY_BYTES = MEGABYTE.

• randCID not reused: worker-test-helpers.ts adds randCID(), but the Contract constructor in ownerSizeTotalWorker.test.ts:28-30 still builds a CID inline. Use this.id = randCID() and drop the createCID import.

• Duplicated rate-limit rule: the startup warning in routes.ts:258-266 re-derives the NODE_ENV rule that signupRateLimitDisabled() already owns. Have that function return the reason, or move the warning into signup-rate-limit.ts.

• startIsolatedServer().stop can leak keys: server-test-helpers.ts:38-41 clears the keys only after stopServer() succeeds. Wrap the call in try { await stopServer() } finally { … nconf.clear … }.

• Inaccurate ESLint message: eslint.config.mjs:38 says "not even in tests", but deno task lint only covers ./src ./scripts (deno.json:4), so test/ is never checked. Drop the clause.

[taoeffect]

LGTM