Skip to content

fix: keep chel serve out of lightweight client mode - #169

Merged
taoeffect merged 4 commits into
masterfrom
lightweight-client-off
Sep 30, 2026
Merged

taoeffect merged 4 commits into
masterfrom
lightweight-client-off

Conversation

@akhileshthite

@akhileshthite akhileshthite commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

Closes okTurtles/libcheloniajs#104
Merges after: okTurtles/libcheloniajs#105

@chelonia/lib is switching to the lightweight client by default
(libcheloniajs#105, merged, not released yet). In that mode its database
selectors keep nothing. chel only swaps in its own storage for persistent
backends, so with mem, the default and what chel init writes, chel serve
would quietly store nothing.

This sets LIGHTWEIGHT_CLIENT=false before the library loads:

  • src/lightweight-client-off.ts sets it, and src/main.ts imports it first.
  • The test and test:symlinks tasks set it too, since the tests load the
    server without going through main.ts.
  • build/main.js is rebuilt. Apart from esbuild renaming its aliases, the only
    change is setting the flag.

It does nothing on the 1.5.0 we pin today, and needs to be in before chel moves
to the next release.

Tested against the new default with the mem backend: a preloaded manifest
comes back 200 with this change and 404 without, from both the source and the
bundle. deno task test passes on Deno 2.9.5.

AI Usage: Claude Opus 5.5 for testing
Self review: Approved

@akhileshthite akhileshthite self-assigned this Sep 29, 2026
@akhileshthite akhileshthite added the bug Something isn't working label Sep 29, 2026
taoeffect
taoeffect previously approved these changes Sep 29, 2026

@taoeffect taoeffect left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right. @corrideat do you concur?

@taoeffect

taoeffect commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

/review


AI review started.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Advanced AI Review

  • Type: Agentic (crush)
  • Model: glm-5.3
Click to expand review

Code Review

Base: origin/master (9ab7321)
Head: 8e68ad23e8093074636a23565a7a0e62cd1477a5 (detached HEAD, "fix: keep chel serve out of lightweight client mode")
Date: 2026-09-29
Model: glm-5.3


The core mechanism is sound. Converting the static imports of parseConfig.ts / utils.ts into top-level await import() calls genuinely defers evaluation of the module graph (which transitively loads @chelonia/lib via commands.ts → serve/server.ts) until after process.env.LIGHTWEIGHT_CLIENT = 'false' runs, and the string 'false' matches the post-libcheloniajs#105 opt-out check (process.env.LIGHTWEIGHT_CLIENT !== 'false') exactly. Verified safe: destructured handlerState is a const object that is mutated in place, never reassigned (src/parseArgs.ts:9-22), so the destructured reference stays live; esbuild (format: 'esm', splitting: false, scripts/build.ts:32-39) preserves lazy dynamic-import semantics and supports top-level await; workers spawned via src/serve/createWorker.ts share process.env with the parent, so they see the variable too.

1. 🟡 test:symlinks still loads @chelonia/lib without LIGHTWEIGHT_CLIENT=false

  • Addressed
  • Dismissed

deno.json:12-13 prefixes the main deno test invocation with LIGHTWEIGHT_CLIENT=false, but the chained deno task test:symlinks runs without it (a per-command env prefix does not survive the &&). That suite is not lib-free: src/migrate-symlinks.test.ts:18 imports ./migrate.ts, and the file's own header comment (lines 21-24) spells out that "importing migrate.ts loads @chelonia/lib, which reads those variables at module scope" — which is why its --allow-env allow-list already includes LIGHTWEIGHT_CLIENT.

Today (pinned @chelonia/lib@1.5.0, where an unset variable means full mode) this is harmless, and the suite only exercises pure path-collision logic (sharedSqliteFilepath). But once the dependency is bumped to the post-#105 default (the very change this PR prepares for), that suite will silently load the lib in lightweight mode while the rest of the test suite runs in full mode — exactly the class of silent divergence this PR exists to prevent.

Suggested fix, keeping both tasks consistent:

"test": "deno task lint && deno task check && LIGHTWEIGHT_CLIENT=false deno test --unstable-worker-options --allow-read=.,$HOME/.cache,$HOME/Library/Caches/deno --allow-write=.,$HOME/.cache,$HOME/Library/Caches/deno --allow-env --allow-ffi --allow-net --allow-sys --allow-run=node,git && LIGHTWEIGHT_CLIENT=false deno task test:symlinks"

(or hoist the variable so it covers both commands).

2. ⚪ Committed build/main.js was not rebuilt for the changed entry module

  • Addressed
  • Dismissed

Recent PRs (#161, #168, #162, #159) each committed a refreshed build/main.js, but this PR changes the entry module src/main.ts while leaving the committed bundle stale: the bundle in build/main.js still contains the old static-import entry and no LIGHTWEIGHT_CLIENT assignment. deno task chel executes build/main.js, so until a rebuild the shipped entry does not match the source.

This self-heals at release time (the npm version hook rebuilds and stages build/, and scripts/publish.ts refuses a mismatched stamp — README "Packaging" step 2), and with @chelonia/lib@1.5.0 pinned the stale bundle behaves identically (an unset variable already selects full mode there, build/main.js:21001). Still, it also means the new top-level-await/dynamic-import entry has not been exercised by an in-repo rebuild; running deno task build in this PR would both refresh the bundle and prove the esbuild + deno bundle pipeline handles the new entry shape.

3. ⚪ process.env.LIGHTWEIGHT_CLIENT silently overrides an operator-set value

  • Addressed
  • Dismissed

src/main.ts:16 unconditionally forces LIGHTWEIGHT_CLIENT = 'false', so LIGHTWEIGHT_CLIENT=true chel serve is silently ignored rather than honored or rejected. This matches what upstream libcheloniajs#104 prescribes ("update chel serve to run with LIGHTWEIGHT_CLIENT='false'") and is arguably correct for a server, so it is likely intended — flagging only so the override is a conscious choice. If it is, no change needed.


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

@akhileshthite

akhileshthite commented Sep 29, 2026 •

Copy link
Copy Markdown
Member Author

/review


AI review started.

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Advanced AI Review

  • Type: Agentic (crush)
  • Model: glm-5.3
Click to expand review

Code Review

Base: origin/master (9ab7321)
Head: d69b0c1d783a80a18780e9ae59e88d908aea177c (HEAD)
Date: 2026-09-29
Model: glm-5.3


Scope note: the change ships three source edits (deno.json, src/lightweight-client-off.ts, src/main.ts) plus the rebuilt build/main.js. Both the source and the committed bundle were reviewed.

Verification performed (no issues found in these areas):

  • Import-order correctness: a side-effect module imported first (src/main.ts:12) is the correct ES-module pattern; commit d69b0c1 rightly replaced commit 8e68ad2's dynamic await import() workaround with it.
  • Bundle ordering: in the rebuilt build/main.js, the flag write process2.env.LIGHTWEIGHT_CLIENT = "false" (line 58833) is top-level and executes before every call site of the inlined lib's lazy init_db() initializer (lines 22183, 62345, 68462-68463), which is where the flag is read (build/main.js:21002). The write therefore wins in the shipped bundle, the deno task chel path, and the compiled binaries.
  • Task env prefixes are not redundant: test files import @chelonia/lib in-process without going through main.ts (e.g. src/migrate-symlinks.test.ts:16-17), so LIGHTWEIGHT_CLIENT=false on the test / test:symlinks tasks covers a path the first-import guard cannot.
  • Permissions: the main.ts shebang and the deno compile flags (scripts/targets.ts:167) both grant unscoped --allow-env, so the env write cannot fail with PermissionDenied in any shipped invocation; test:symlinks's restricted --allow-env allowlist already includes LIGHTWEIGHT_CLIENT.
  • No collateral damage: the workers (src/serve/creditsWorker.ts, src/serve/ownerSizeTotalWorker.ts) do not import @chelonia/lib; the dashboard build's own LIGHTWEIGHT_CLIENT define (scripts/dashboard-esbuild.ts:20,110, default 'true' for the browser client) is intentionally outside this override's scope and is unaffected.
  • Style: no-semicolons / single quotes / 2-space indent match the project conventions.

1. ⚪ The forced flag is a no-op against the currently pinned @chelonia/lib

  • Addressed
  • Dismissed

deno.json:60 pins npm:@chelonia/lib@1.5.0/, and in 1.5.0 lightweight mode is opt-in: the inlined dist reads dbPrimitiveSelectors = process.env.LIGHTWEIGHT_CLIENT === "true" ? { … } (build/main.js:21002), so an unset variable already means "keep everything". The comment added in src/lightweight-client-off.ts:3-6 ("Since okTurtles/libcheloniajs#104 it defaults to a lightweight client that keeps nothing") describes the future lib from the companion PR, not the one actually vendored here.

This is not a bug — the guard is forward-compatible and the PR explicitly sequences after libcheloniajs#105 — but two practical consequences are worth being aware of:

  1. Nothing in this PR can exercise the fix until the dependency bump lands; the CI signal for it is only "tests still pass with a no-op".
  2. A reader at this pin may look for lightweight behavior that does not exist yet. A one-word hedge ("Since Make LIGHTWEIGHT_CLIENT the default libcheloniajs#104 it will default…", or "as of the lib version that follows chel.toml validation #105") would keep the comment accurate at both pins.

2. ⚪ Guard only exists at the CLI entry and in deno tasks; direct importers of server modules stay unguarded

  • Addressed
  • Dismissed

The protection lives in exactly two places: src/main.ts:12 (CLI entry) and the LIGHTWEIGHT_CLIENT=false prefixes on the test / test:symlinks tasks (deno.json:12-13). Once the lightweight-default lib lands, any other in-process path that loads @chelonia/lib — e.g. a developer running deno test src/serve/routes-stateless.test.ts by hand instead of via deno task test, or a script importing src/serve/database.ts directly — will silently initialize the lib in lightweight mode, with confusing failures (or worse, silent no-op storage) that do not reproduce under the task runner.

If that risk feels worth closing, the server's own DB entry point could carry the guard too, making every path that loads the server's storage layer safe regardless of entry:

// src/serve/database.ts — must stay the first import, like in main.ts
import '../lightweight-client-off.ts'
import 'npm:@chelonia/lib/chelonia'
import 'npm:@chelonia/lib/db'
// …

Filed at low confidence: the entry-point approach is a deliberate choice documented in the module comment, it does cover every shipped artifact (CLI, bundle, binaries, task-driven tests), and the lib's module-scope read makes any later process.env assignment ineffective, so partially-placed guards could breed a false sense of coverage.


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

Comment thread src/main.ts Outdated
Comment on lines +11 to +12
// Has to stay the first import. See the module for why.
import './lightweight-client-off.ts'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a space between lines 12 and 13. This keeps tools from accidentally changing the order (e.g., if one uses Organize imports in VS Code).

Comment thread src/main.ts Outdated
// Third-party modules:
// https://deno.land/x

// Has to stay the first import. See the module for why.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a brief explanation apart from See the module for why.. For example: Has to stay the first import. The server always has to store messages. See the module for more details.

corrideat
corrideat previously approved these changes Sep 30, 2026

@corrideat corrideat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with two small comments.

@taoeffect

Copy link
Copy Markdown
Member

@chelonia/lib@2.0.0 has been published, could you please update this PR to use it?

Comment thread build/main.js Dismissed

@taoeffect taoeffect left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@taoeffect
taoeffect merged commit f42cb78 into master Sep 30, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make LIGHTWEIGHT_CLIENT the default

4 participants