Report version from manifest

[corrideat]

These changes read version information from chelonia.json instead of from environment variables and also present per-contract versions (as opposed to a single, global, CONTRACTS_VERSION)

[corrideat]

/crush_fast

---

AI review started.

[Copilot]

Pull request overview

This PR switches backend version reporting to read from chelonia.json instead of environment variables, and updates the pinning workflow to surface/store per-contract version metadata.

Changes:

• Load chelonia.json at server startup and send VERSION_INFO with appVersion and per-contract versions.
• Update pin command output and config lookup to use a “full” (unsanitized) contract name.

Reviewed changes

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

File	Description
src/serve/server.ts	Imports chelonia.json and changes the VERSION_INFO payload to report app + per-contract versions.
src/pin.ts	Adjusts manifest parsing/printing to include fullContractName and uses it when reading/writing chelonia.json contract entries.

---

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

[Copilot]

Using join(process.cwd(), 'chelonia.json') as an import() specifier is not portable (e.g., backslashes on Windows) and can fail because import() expects a valid module specifier/URL. Consider converting the filesystem path to a file:// URL (e.g., via pathToFileURL) before importing.

[github-actions]

Advanced AI Review

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

Click to expand review

Now let me also check one more thing about the pin.ts changes — the directory structure uses contractName (sanitized) for paths but fullContractName (unsanitized) for config keys:Now let me check the updateCheloniaConfig function more carefully - it receives fullContractName but the path is built using contractName:

Looking at line 77: await updateCheloniaConfig(fullContractName, manifestVersion, manifestPath) — and at line 201: const pinnedManifestPath = contracts/${contractName}/${version}/${manifestFileName}`` — but contractName in `updateCheloniaConfig` is actually the `fullContractName` parameter! The directory path is constructed from the unsanitized name.

Let me verify this:Now I have all the context I need. Here is my review:

---

Issue 1: 🔴 import() returns module namespace, not raw JSON — appVersion and contracts will always be undefined/empty

• Addressed
• Dismissed

src/serve/server.ts:37-45 and src/serve/server.ts:437-441

The dynamic import() of a JSON file returns a module namespace object (i.e., { default: { ... }, [Symbol.toStringTag]: "Module" }), not the raw JSON. So cheloniaAppManifest will be { default: { contracts: { ... } } }, meaning:

• cheloniaAppManifest?.appVersion → always undefined (falls back to null)
• cheloniaAppManifest?.contracts → always undefined (falls back to {})

Version info sent to every connecting client will always be:

{ "appVersion": null, "contractsVersion": {} }

Fix — access via .default:

const cheloniaAppManifest = await (async () => {
  try {
    const mod = await import(join(process.cwd(), 'chelonia.json'), {
      with: { type: 'json' }
    })
    return mod.default
  } catch {
    console.warn('`chelonia.json` not found. Version information will be unavailable.')
  }
})()

Or alternatively, change the access sites:

appVersion: cheloniaAppManifest?.default?.appVersion || null,
contractsVersion: Object.fromEntries(
  Object.entries(cheloniaAppManifest?.default?.contracts || {})
    .map(([k, v]) => [k, (v as Record<string, number | string>).version])
)

---

Issue 2: 🟡 Success message uses version (may be undefined) instead of manifestVersion

• Addressed
• Dismissed

src/pin.ts:79

console.log(colors.green(`✅ Successfully pinned ${contractName} to version ${version}`))

version is args['manifest-version'] (line 19) and is optional (demandOption: false at line 238). When the user doesn't supply it, this prints:

✅ Successfully pinned chatroom to version undefined

The actual pinned version is manifestVersion. This is also inconsistent with lines 53–62 which correctly use manifestVersion for all other logic.

Fix:

console.log(colors.green(`✅ Successfully pinned ${contractName} to version ${manifestVersion}`))

---

Issue 3: 🟡 updateCheloniaConfig receives fullContractName (unsanitized) and uses it for directory paths

• Addressed
• Dismissed

src/pin.ts:77 and src/pin.ts:199-205

At line 77, updateCheloniaConfig is called with fullContractName:

await updateCheloniaConfig(fullContractName, manifestVersion, manifestPath)

Inside updateCheloniaConfig at line 201, the contractName parameter (which is actually fullContractName) is used to build a path written into the config:

const pinnedManifestPath = `contracts/${contractName}/${version}/${manifestFileName}`

This is a config path mismatch with the actual filesystem layout. The actual files are copied using the sanitized contractName (line 76: copyContractFiles(contractFiles, manifestPath, contractName, ...)), which means:

• Actual file location: contracts/<sanitized-name>/<version>/...
• Config path value: contracts/<full-raw-name>/<version>/...

If the raw contract name contains characters like /, \, :, etc., the config path will point to a nonexistent location.

Fix — pass the sanitized contractName to updateCheloniaConfig and use the fullContractName only as the config key:

await updateCheloniaConfig(fullContractName, contractName, manifestVersion, manifestPath)

And update the function signature:

async function updateCheloniaConfig (
  configKey: string,
  dirName: string,
  version: string,
  manifestPath: string
) {
  const manifestFileName = basename(manifestPath)
  const pinnedManifestPath = `contracts/${dirName}/${version}/${manifestFileName}`

  cheloniaConfig.contracts[configKey] = {
    version,
    path: pinnedManifestPath
  }
  // ...
}

---

Issue 4: 🟡 manifestVersion is not sanitized before use in path construction

• Addressed
• Dismissed

src/pin.ts:65 and src/pin.ts:115

const contractVersionDir = join(projectRoot, 'contracts', contractName, manifestVersion)

const versionDir = join(projectRoot, 'contracts', contractName, version)

manifestVersion comes from body.version in the manifest JSON (line 92). While contractName is sanitized via sanitizeContractName, manifestVersion has no validation. A malicious manifest could contain "version": "../../etc", and since join(projectRoot, 'contracts', contractName, '../../etc') resolves to projectRoot/etc, this could result in directory creation outside the intended contracts/ tree.

Note: join() does not resolve .. on its own (unlike resolve()), but mkdir with { recursive: true } will create the full path, and existsSync would check the correct resolved path. The real risk is moderate since this requires a maliciously crafted manifest file, but a similar sanitizeContractName-style check should be applied.

Fix — add validation after parsing:

if (/\.\./.test(manifestVersion) || /[/\\]/.test(manifestVersion)) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

---

Issue 5: ⚪ cheloniaAppManifest loaded at module top-level may throw on bundling or break hot reload

• Addressed
• Dismissed

src/serve/server.ts:37-45

const cheloniaAppManifest = await (async () => {
  try {
    return await import(join(process.cwd(), 'chelonia.json'), {
      with: { type: 'json' }
    })
  } catch {
    console.warn('`chelonia.json` not found. Version information will be unavailable.')
  }
})()

This is a top-level await in a module that gets bundled by esbuild. The import() path is dynamic (join(process.cwd(), ...)), so it cannot be resolved at build time. While the try/catch prevents crashes, this means:

1. The import will be evaluated once at server startup and cached by the module system. If chelonia.json is updated while the server is running (e.g., via chel pin in dev mode with --dev watch), the server will serve stale version info until restarted.
2. The console.warn fires silently during startup with no indication of which directory was searched.

This is a minor concern (the previous process.env approach had the same staleness issue), but worth noting for the --dev mode workflow described in the README.

---

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

[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 additional context around the manifest version regex issue and how version info is consumed.Now I have the full context. Let me verify one more thing about the worker cleanup:Now I have all the context needed for a thorough review.

---

Code Review: Report Version from Manifest

1. 🔴 backend/server/stop no longer terminates workers or clears intervals — process will hang on shutdown

• Addressed
• Dismissed

src/serve/server.ts:258-260

The previous implementation of backend/server/stop called ownerSizeTotalWorker?.terminate(), creditsWorker?.terminate(), and clearInterval(pushHeartbeatIntervalID). The new version only calls hapi.stop():

// NEW — missing cleanup
'backend/server/stop': function () {
    return hapi.stop()
  },

Meanwhile, two long-lived workers (ownerSizeTotalWorker and creditsWorker) are created at src/serve/server.ts:56-61 and a setInterval is created at src/serve/server.ts:599 — but neither the workers nor the interval are terminated/cleared on shutdown. Since backend/server/stop is called during the SERVER_EXITING flow in src/serve/index.ts:48, the process will likely hang because the worker threads and interval keep the event loop alive.

Suggestion:

'backend/server/stop': async function () {
    await hapi.stop()
    await Promise.all([
      ownerSizeTotalWorker?.terminate(),
      creditsWorker?.terminate()
    ])
  },

The setInterval for the push heartbeat should also have its ID stored so it can be cleared during shutdown (see issue #2).

---

2. 🟡 Push heartbeat setInterval is no longer clearable

• Addressed
• Dismissed

src/serve/server.ts:599

// NEW — interval ID not captured
setInterval(() => {

Previously, the interval ID was stored in pushHeartbeatIntervalID and cleared in backend/server/stop. Now the ID is discarded, so this interval runs forever and cannot be cleaned up. Combined with issue #1, this contributes to the process hanging on shutdown.

Suggestion: Store the interval ID and clear it in backend/server/stop:

const pushHeartbeatIntervalID = setInterval(() => {
    // ...
  }, 1 * 60 * 60 * 1000)

Then in backend/server/stop:

'backend/server/stop': async function () {
    clearInterval(pushHeartbeatIntervalID)
    await hapi.stop()
    await Promise.all([
      ownerSizeTotalWorker?.terminate(),
      creditsWorker?.terminate()
    ])
  },

---

3. 🟡 Global regex used with .test() — stateful lastIndex anti-pattern

• Addressed
• Dismissed

src/pin.ts:9 and src/pin.ts:43

const RESERVED_FILE_CHARS = /[/\\:*?"<>|]/g   // line 9 — 'g' flag

// ...

if (RESERVED_FILE_CHARS.test(manifestVersion)) {   // line 43 — .test() on global regex
  exit(`Invalid manifest version: ${manifestVersion}`)
}

A regex with the g flag maintains stateful lastIndex between .test() calls. While the current code flow happens to work (because sanitizeContractName at line 17 calls .replace() with this regex, which resets lastIndex to 0 before .test() runs), this is fragile. If the call order ever changes — or if someone adds another .test() call elsewhere — the validation could silently pass for strings containing forbidden characters.

Suggestion: Use a separate non-global regex for validation:

const RESERVED_FILE_CHARS = /[/\\:*?"<>|]/g
const RESERVED_FILE_CHARS_TEST = /[/\\:*?"<>|]/

function sanitizeContractName (contractName: string): string {
  return contractName.replace(RESERVED_FILE_CHARS, '_').replace(/\.\./g, '__')
}

// ...
if (RESERVED_FILE_CHARS_TEST.test(manifestVersion)) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

Or alternatively, don't extract the constant and keep inline regexes — each literal creates a fresh regex with lastIndex = 0.

---

4. 🟡 Overly broad catch swallows all errors with misleading message

• Addressed
• Dismissed

src/serve/server.ts:38-46

const cheloniaAppManifest = await (async () => {
  try {
    return (await import(pathToFileURL(join(process.cwd(), 'chelonia.json')).toString(), {
      with: { type: 'json' }
    })).default
  } catch {
    console.warn('`chelonia.json` not found. Version information will be unavailable.')
  }
})()

This catches all errors — JSON parse errors, permission errors, encoding errors — and reports them as "chelonia.json not found." If the file exists but is malformed, the operator would see the wrong diagnostic and waste time looking in the wrong direction.

Suggestion: Differentiate error types:

const cheloniaAppManifest = await (async () => {
  try {
    return (await import(pathToFileURL(join(process.cwd(), 'chelonia.json')).toString(), {
      with: { type: 'json' }
    })).default
  } catch (e) {
    const code = (e as { code?: string })?.code
    if (code === 'ERR_MODULE_NOT_FOUND' || code === 'ENOENT') {
      console.warn('`chelonia.json` not found. Version information will be unavailable.')
    } else {
      console.warn('Error loading `chelonia.json`. Version information will be unavailable.', e)
    }
  }
})()

---

5. ⚪️ contractsVersion returns {} instead of null for empty contracts object

• Addressed
• Dismissed

src/serve/server.ts:439-442

contractsVersion: cheloniaAppManifest?.contracts ? Object.fromEntries(
  Object.entries(cheloniaAppManifest?.contracts)
    .map(([k, v]) => [k, (v as Record<string, number | string>).version])
) : null

If cheloniaAppManifest.contracts is {} (truthy but empty), the result is {} instead of null. The old code returned null when CONTRACTS_VERSION was not set. Downstream clients that distinguish between null and {} would see different behavior.

Suggestion: Add an emptiness check:

contractsVersion: cheloniaAppManifest?.contracts && Object.keys(cheloniaAppManifest.contracts).length > 0
  ? Object.fromEntries(
      Object.entries(cheloniaAppManifest.contracts)
        .map(([k, v]) => [k, (v as { version: string }).version])
    )
  : null

---

6. ⚪️ Unsafe type cast in contracts version mapping

• Addressed
• Dismissed

src/serve/server.ts:441

.map(([k, v]) => [k, (v as Record<string, number | string>).version])

The as Record<string, number | string> cast bypasses type safety. If a chelonia.json entry has an unexpected shape (e.g., missing version or with version as an object), this silently produces undefined or an unexpected value that gets sent to WebSocket clients.

Suggestion: Use a more precise type assertion and add a fallback:

.map(([k, v]) => [k, (v as { version: string })?.version ?? null])

---

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

[devin-ai-integration]

✅ Devin Review: No Issues Found

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

[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.

[github-actions]

Advanced AI Review

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

Click to expand review

Code Review: Report version from manifest

1. 🔴 Breaking wire protocol change in VERSION_INFO notification

• Addressed
• Dismissed

src/serve/server.ts:437-442

The field names in the VERSION_INFO WebSocket notification have changed from GI_VERSION / CONTRACTS_VERSION to appVersion / contractsVersion, and the type of the contracts version changed from a single string to an object (map of contract names to versions). Any existing client listening for this notification will silently fail to match versions:

// Old payload (what clients expect):
{ GI_VERSION: "1.0.0", CONTRACTS_VERSION: "2.0.0" }

// New payload (what server now sends):
{ appVersion: "1.0.0", contractsVersion: { "gi.contracts_chatroom": "2.0.6", ... } }

If this is intentional and the consumers are being updated in lockstep, consider adding a comment noting the breaking change. If not, this will cause the version-mismatch detection and auto-refresh logic on the client side to break silently (comparing undefined !== undefined would evaluate to false, so the client might not detect real version changes).

Suggestion: If backward compatibility is needed, send both old and new field names during a transition period:

const contractsVersionMap = cheloniaAppManifest?.contracts
  ? Object.fromEntries(
      Object.entries(cheloniaAppManifest.contracts)
        .map(([k, v]) => [k, (v as { version: string }).version])
    )
  : null

const versionInfo = {
  appVersion: cheloniaAppManifest?.appVersion || null,
  contractsVersion: contractsVersionMap,
  // Legacy fields for backward compatibility
  GI_VERSION: cheloniaAppManifest?.appVersion || null,
  CONTRACTS_VERSION: contractsVersionMap
    ? Object.values(contractsVersionMap).join(', ')
    : null
}

---

2. 🟡 chelonia.json is loaded from process.cwd() instead of the served app directory

• Addressed
• Dismissed

src/serve/server.ts:40

return (await import(pathToFileURL(join(process.cwd(), 'chelonia.json')).toString(), {

The [directory] CLI argument to chel serve is stored as server:appDir in nconf and used by routes.ts to resolve static assets. But chelonia.json is loaded from process.cwd(), which may not be the same directory. If a user runs chel serve /path/to/app from a different working directory, the wrong chelonia.json (or none) will be loaded.

Suggestion: Use the configured app directory:

import nconf from 'npm:nconf'

// After nconf is configured with CLI args:
const appDir = nconf.get('server:appDir') || process.cwd()
const cheloniaAppManifest = await (async () => {
  try {
    return (await import(pathToFileURL(join(appDir, 'chelonia.json')).toString(), {
      with: { type: 'json' }
    })).default
  } catch {
    console.warn('`chelonia.json` unparsable or not found. Version information will be unavailable.')
  }
})()

Note: this would require moving the import after nconf is configured, or deferring the load to the connection handler.

---

3. 🟡 Inconsistent contract name in final success log message

• Addressed
• Dismissed

src/pin.ts:89

All log messages in the function were updated to use fullContractName (the raw name from the manifest), but the final success message still uses contractName (the sanitized filesystem name):

console.log(colors.green(`✅ Successfully pinned ${contractName} to version ${manifestVersion}`))
console.log(colors.gray(`Location: contracts/${contractName}/${manifestVersion}/`))

If fullContractName is gi.contracts/chatroom and contractName is gi.contracts_chatroom, the user would see the sanitized name in the success message but the original name everywhere else.

Suggestion:

console.log(colors.green(`✅ Successfully pinned ${fullContractName} to version ${manifestVersion}`))
console.log(colors.gray(`Location: contracts/${contractName}/${manifestVersion}/`))

---

4. 🟡 chelonia.json config key changed from sanitized name to full contract name — no migration

• Addressed
• Dismissed

src/pin.ts:60 and src/pin.ts:187

The config now stores contracts under fullContractName (raw manifest name) instead of contractName (sanitized):

// Line 60: reading
const currentPinnedVersion = cheloniaConfig.contracts[fullContractName]?.version

// Line 187: writing
cheloniaConfig.contracts[fullContractName] = {

Existing chelonia.json files that use sanitized keys (e.g., "gi.contracts_chatroom") will not be matched by the new code looking up the raw name (e.g., "gi.contracts/chatroom"). This means existing pinned versions won't be detected, and the user could be prompted to re-pin or could end up with duplicate entries under both the old and new keys.

Suggestion: Add a migration step that scans for and updates old sanitized keys, or at minimum document this as a breaking change in the config format.

---

5. ⚪️ Unsafe type assertion in contracts version mapping

• Addressed
• Dismissed

src/serve/server.ts:441

.map(([k, v]) => [k, (v as { version: string }).version])

The as { version: string } assertion provides no runtime safety. If chelonia.json has a malformed entry (e.g., { "contracts": { "foo": {} } }), the .version access would return undefined, which would silently appear in the version map.

Suggestion:

contractsVersion: cheloniaAppManifest?.contracts ? Object.fromEntries(
  Object.entries(cheloniaAppManifest.contracts)
    .filter(([, v]) => typeof v === 'object' && v !== null && typeof v.version === 'string')
    .map(([k, v]) => [k, (v as { version: string }).version])
) : null

---

6. ⚪️ Top-level await with dynamic import may fail at build/bundle time

• Addressed
• Dismissed

src/serve/server.ts:38-46

The top-level await with a dynamic import() of a JSON file is Deno-specific and may not play well with the esbuild bundling process described in AGENTS.md. If the bundler does not support top-level await, this will cause build failures. Additionally, import(..., { with: { type: 'json' } }) is an import assertion that requires Deno 1.x+ or Node 22+. Worth verifying this works through the full build pipeline (deno task build / deno task dist).

---

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

[taoeffect]

Looks solid, I think! Just have a documentation request:

Where is chelonia.json documented?

Need to know:

• What it is
• What it's for
• What it can contain
• How it's different from the .toml config

Could you add this to the README.md maybe?

[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.

[github-actions]

Advanced AI Review

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

Click to expand review

Let me check a few more things for completeness.Now I have enough context to provide a thorough review.

---

Code Review: Report version from manifest

Issue 1: 🔴 Path traversal via manifestVersion in pin.ts

• Addressed
• Dismissed

The RESERVED_FILE_CHARS regex at src/pin.ts:9 does not include .. (dot-dot):

const RESERVED_FILE_CHARS = /[/\\:*?"<>|]/

The validation at src/pin.ts:44-46 therefore does not catch a manifestVersion of "..":

if (RESERVED_FILE_CHARS.test(manifestVersion)) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

Yet manifestVersion is used directly in filesystem path construction at src/pin.ts:73:

const contractVersionDir = join(projectRoot, 'contracts', contractName, manifestVersion)

Since path.join('/app', 'contracts', 'foo', '..') resolves to /app/contracts, a crafted manifest with "version": ".." would cause contract files to be written to the parent contracts/ directory instead of a versioned subdirectory. This is a directory-level path traversal.

Contrast this with sanitizeContractName (src/pin.ts:17-18) which explicitly handles ..:

function sanitizeContractName (contractName: string): string {
  return contractName.replace(RESERVED_FILE_CHARS_REPLACE, '_').replace(/\.\./g, '__')
}

Suggested fix — add .. rejection to the version validation, and also guard against null bytes and empty strings:

if (!manifestVersion || RESERVED_FILE_CHARS.test(manifestVersion) || manifestVersion.includes('..') || manifestVersion.includes('\0')) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

Or alternatively, apply the same sanitization approach used for contractName:

const safeVersion = manifestVersion.replace(RESERVED_FILE_CHARS_REPLACE, '_').replace(/\.\./g, '__').replace(/\0/g, '')
if (safeVersion !== manifestVersion) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

---

Issue 2: 🔴 Path traversal via contractFiles.main / contractFiles.slim in pin.ts

• Addressed
• Dismissed

The file names body.contract.file and body.contractSlim?.file are taken directly from the manifest body with no sanitization at src/pin.ts:101-102:

const mainFile = body.contract.file
const slimFile = body.contractSlim?.file

These are then used in join() calls at src/pin.ts:140-147:

const mainSource = join(sourceDir, contractFiles.main)
const mainTarget = join(targetDir, contractFiles.main)
// ...
const slimSource = join(sourceDir, contractFiles.slim)
const slimTarget = join(targetDir, contractFiles.slim)

A manifest containing "contract": {"file": "../../etc/passwd"} would cause copyFile to read from and write to paths outside the intended directories. While manifests are typically created by chel manifest (which uses path.basename()), pin.ts does not enforce this when consuming them, and manifests could come from untrusted sources.

Suggested fix — validate that the file names are basenames only:

const mainFile = body.contract.file
const slimFile = body.contractSlim?.file

if (!mainFile || mainFile !== basename(mainFile) || mainFile.includes('..')) {
  exit(`Invalid contract file path: ${mainFile}`)
}
if (slimFile && (slimFile !== basename(slimFile) || slimFile.includes('..'))) {
  exit(`Invalid slim file path: ${slimFile}`)
}

---

Issue 3: 🟡 Undefined version property silently produces malformed WebSocket data in server.ts

• Addressed
• Dismissed

At src/serve/server.ts:439-442, the code casts each config entry as { version: string } without runtime validation:

contractsVersion: cheloniaAppManifest?.contracts ? Object.fromEntries(
  Object.entries(cheloniaAppManifest?.contracts)
    .map(([k, v]) => [k, (v as { version: string }).version])
) : null

The as keyword is a TypeScript assertion with no runtime check. If chelonia.json has an entry missing the version field:

{
  "contracts": {
    "gi.contracts/chatroom": {
      "path": "contracts/gi.contracts_chatroom/2.0.6/chatroom.manifest.json"
    }
  }
}

Then (v as { version: string }).version evaluates to undefined. Object.fromEntries will include it as [["gi.contracts/chatroom", undefined]], which when serialized via JSON.stringify in createNotification becomes {"gi.contracts/chatroom": null} — silently dropping data that clients may depend on.

Suggested fix — filter out malformed entries or provide a fallback:

contractsVersion: cheloniaAppManifest?.contracts ? Object.fromEntries(
  Object.entries(cheloniaAppManifest.contracts)
    .filter(([, v]) => v && typeof v.version === 'string')
    .map(([k, v]) => [k, (v as { version: string }).version])
) : null

---

Issue 4: 🟡 manifestVersion not validated for null bytes or control characters in pin.ts

• Addressed
• Dismissed

The RESERVED_FILE_CHARS regex at src/pin.ts:9 does not include \0 (null byte) or other control characters:

const RESERVED_FILE_CHARS = /[/\\:*?"<>|]/

A manifestVersion containing \0 (e.g. "foo\0bar") would pass validation at src/pin.ts:44-46. Null bytes in filesystem paths can cause unexpected behavior — many Node.js filesystem functions truncate at \0, which could result in creating a directory named "foo" instead of "foo\0bar".

Additionally, manifestVersion is interpolated directly into multiple console.log/console.error calls (lines 49, 53, 54, 63, 68, 70, 77, 87) without sanitization. A version string containing newlines or ANSI escape sequences could produce misleading log output (log injection).

Suggested fix — extend the regex to reject null bytes and validate against a whitelist pattern:

const VALID_VERSION = /^[a-zA-Z0-9._+-]+$/

if (!manifestVersion || !VALID_VERSION.test(manifestVersion)) {
  exit(`Invalid manifest version: ${manifestVersion}`)
}

---

Issue 5: ⚪ cheloniaAppManifest loaded via top-level await with dynamic import() may behave differently in bundled vs. unbundled contexts

• Addressed
• Dismissed

At src/serve/server.ts:38-46:

const cheloniaAppManifest = await (async () => {
  try {
    return (await import(pathToFileURL(join(process.cwd(), 'chelonia.json')).toString(), {
      with: { type: 'json' }
    })).default
  } catch {
    console.warn('`chelonia.json` unparsable or not found. Version information will be unavailable.')
  }
})()

The import() with { with: { type: 'json' } } uses the import assertions / import attributes syntax. Two concerns:

1. The with: { type: 'json' } syntax (import attributes) was stabilized in Node.js 18.20+ / 20.10+. If the build targets older Node.js versions, this may fail at runtime (though the catch block would handle it gracefully, resulting in silent null).

2. After the esbuild bundling step (described in AGENTS.md), the dynamic import() of a path computed at runtime from process.cwd() may not work as expected in the bundled output — esbuild typically cannot resolve fully dynamic imports. This is likely fine since the build output runs in Node.js (not a browser), but it's worth verifying that the bundled build/main.js can still perform this dynamic JSON import correctly.

This is a low-confidence concern because the catch block provides graceful degradation.

---

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.

[taoeffect]

LGTM! 👍

[taoeffect]

Oh, actually, I missed one thing:

[taoeffect]

Question: does this design assume there will only be one app per server?

If so, that's clearly not a safe assumption (per the other open issues).

So I think we should address this somehow in this PR in a way that either fully addresses it, or sets up future versions to address it properly.

I.e.:

• It could fully address this by returning all apps
• It could partially address this by passing in some server API version perhaps that clients could distinguish for figuring out what format to expect?

[corrideat]

At the moment there can only possibly be one app per server. Since this is being sent when a WS connection is established, I think this is fine (since presumably, a WS client will be for a single app).

I think it's low risk to make this assumption since, whichever changes are needed, they are limited to the server API and will not require changing contracts.

In addition, presumably, we'll want https://groupincome.app (as an example) to be a single app, regardless of how many apps the backend is running. I.e., the origin will identify the app.

With this in mind, these changes look safe and perhaps even future proof:

1. We might need to refactor chel to serve multiple apps, but those changes don't seem related to the ones here (it'd mean, in this case, that we'd have to pick which cheloniaAppManifest we want --- now, there's a single choice)
2. In the future, since we need to pick a way of choosing the cheloniaAppManifest (see previous point), we need a way to do it. If we do it based on origin, this already is future proof. If we do it some other way, we can change the WS protocol to add an app id selector (which would not require recreating contracts)

[corrideat]

Question: does this design assume there will only be one app per server?

Currently, yes. But, I think little if anything will change from the client's perspective.