ReviewOS

stacks/uplink

Use the framework's config and auto-import conventions instead of Uplink's own

#40
Open glennmichael123 opened this 9 days ago ·
9 days ago

Uplink is a Stacks application but keeps its own config shape and its own module layout, so it does not benefit from what the framework already provides. Two moves fix that: config files belong in ./config, and anything app code calls belongs where auto-imports can reach it.

Both have a trap that is invisible until runtime. Each is written out below, verified against the framework source rather than inferred.


1. Config

What will not work

Moving app/Uplink/config.ts to config/uplink.ts and expecting config.uplink from @stacksjs/config gives undefined.

@stacksjs/config does not scan config/. It walks a hardcoded list of about 45 [configKey, filename] pairs in storage/framework/core/config/src/overrides.ts (the userConfigs array), loads only those files, and merges them over the framework defaults. uplink is not in that list, and should not be added: that list is the framework's own config surface, not a registry for application config.

This failure is quiet. config.uplink is simply undefined, and the usual if (config.uplink?.pollMs) reads as "not configured" rather than as an error.

What to use instead

bunfig, which is what the Stacks family already uses for exactly this. stx resolves its own config the same way, with { name: 'stx', alias: 'ui' }.

import type { UplinkConfig } from './types'
import { loadConfig } from 'bunfig'

export const uplink = await loadConfig<UplinkConfig>({
  name: 'uplink',
  defaultConfig: { /* today's defaults, unchanged */ },
  checkEnv: true,
})
  • defaultConfigDir is resolve(process.cwd(), 'config'), so name: 'uplink' reads config/uplink.ts with no path wiring.
  • It deep-merges over defaultConfig and is documented to always return at least the defaults, never null or undefined, so there is no "config missing" branch to write.
  • The config file itself is written the way every Stacks config file is written, which is also what makes it typed in an editor:
// config/uplink.ts
import type { UplinkConfig } from '../app/Uplink/types'

export default {
  allowed: [],
  pollMs: 2000,
  // ...
} satisfies UplinkConfig

Env vars come free, and replace hand-rolled reads

With checkEnv: true, bunfig's applyEnvVarsToConfig derives the variable name from the config name plus the key path: the prefix is name.toUpperCase(), and each camelCase segment is split on capitals and upper-cased. So pollMs becomes UPLINK_POLL_MS, maxChars becomes UPLINK_MAX_CHARS, ackAfterMs becomes UPLINK_ACK_AFTER_MS. A legacy flat form (UPLINK_POLLMS) is also accepted.

That subsumes the manual UPLINK_* handling in app/Uplink/settings.ts, which is the "different way of config loading" worth removing. Check the derived names against the ones already documented in .env.example before switching, and keep any that differ as explicit overrides rather than renaming what users have set.

One dependency note

bunfig ships inside better-dx, which uplink currently has as a devDependency. This is a runtime import, and the Stacks convention in AGENTS.md is explicit about that case: a package that imports one of better-dx's tools at runtime declares it as a real dependency. So add bunfig to dependencies rather than relying on the dev bundle.

The trap, for framework config specifically

This one applies to reads of @stacksjs/config, not to bunfig. config.<key> returns framework defaults until await overridesReady resolves, because the override object is mutated in place asynchronously to avoid a module cycle. Any framework config uplink reads at module scope in a background process gets defaults, not the application's values. If uplink reads config.app, config.logging or similar, await overridesReady first.


2. Auto-imports

What is actually injected

On the server, injectGlobalAutoImports() puts these on globalThis:

  • every model,
  • everything exported from app/Jobs/ and app/Controllers/,
  • everything exported from resources/functions/, plus the framework defaults' functions.

The generated declaration is storage/framework/types/server-auto-imports.d.ts. Regenerate with buddy generate (--types for the declarations). A new file added without regenerating is a typecheck failure, not a silent one, so this is the cheap half.

So Chris is right that resources/functions/ is the place for support code app code calls. Class exports are fine; the barrel re-exports whatever the module exports.

The trap, and it is the important part of this issue

A declaration is not an injection. The globals exist only after injectGlobalAutoImports() runs, and that call happens at server boot (buddy serve, storage/framework/core/buddy/src/production-server.ts). It is also guarded to run once per process.

Uplink's engine does not run there. It runs in a launchd background service and in app/Commands/Uplink.ts. If that process never calls injectGlobalAutoImports(), then a bare CodexEngine(...) in service code typechecks and throws ReferenceError at runtime, because the ambient declaration exists while the runtime binding does not.

This is not hypothetical. It is stacksjs/stacks#2585: a declaration surface that named symbols nothing injected, which typechecked cleanly and took whole pages down unhydrated in production. The fix there was to stop declaring what is not injected.

So pick one deliberately:

  • Either have the service boot through the path that injects, and verify it,
  • or keep explicit imports in service code and use auto-imports only where code runs inside the server: routes, actions, jobs, the dashboard.

Explicit imports are not a workaround here. They are what the framework itself does, and AGENTS.md says so: the built-in actions and models import Action, route, response, schema and the rest from their packages even though those names are declared as globals.

Verify at runtime, not in the editor

After moving anything, prove the binding exists in the process that uses it:

  1. buddy generate --types
  2. confirm the name appears in storage/framework/types/server-auto-imports.d.ts
  3. in the service process, log typeof (globalThis as any).CodexEngine once at startup

Step 3 is the one that catches the failure above. Steps 1 and 2 pass in the broken case.

Two smaller rules

  • Name collisions are skipped, not reported. A name that would shadow a built-in is not injected. This is why the framework's Error and Request models must be imported directly. Avoid exporting a name like Request, Response or Error from resources/functions/.
  • Browser side is a different surface. stx templates get what the stx runtime attaches to window. storage/framework/browser-auto-imports.json is compile-time only and does not determine what the browser has, which is the other half of #2585. A TypeScript module imported by an stx client script must import everything explicitly, because entry bindings do not leak into bundled module scope. Relevant to app/Uplink/dashboard.ts and app/Uplink/menubar.html.

Suggested split for the current files

  • app/Uplink/config.ts → values move to config/uplink.ts; the UplinkConfig interface stays in a types module and the config file satisfies it.
  • app/Uplink/settings.ts → the UPLINK_* reading collapses into bunfig's checkEnv.
  • codex-engine.ts, engine.ts, format.ts, filter.ts and similar → resources/functions/, subject to the runtime check above.
  • messages-db.ts, typedstream.ts, sqlite-store.ts and the rest of the service plumbing can stay where they are. Auto-import is for what app code reaches for casually; making every internal module a global costs namespace and gains nothing.

Related: #4, #5, #6.

9 days ago

Both halves are now verified against the installed packages rather than inferred. The config half has landed; the auto-import half turns out to be a decision rather than a move, and the reasoning is below so it does not get re-litigated later.

Config: done in #42, with one deliberate deviation

config/uplink.ts now holds the defaults and app/Uplink/config.ts reads them. The UPLINK_* handling that was hand-rolled in settings.ts is gone, replaced by bunfig, and no variable was renamed: all seventeen names bunfig derives already match .env.example, so existing .env files are untouched.

What this issue proposed does not work here. loadConfig({ name: 'uplink', checkEnv: true }) applies the environment to defaultConfig and then merges the config file over the result (bunfig/dist/index.js:2303). Precedence is therefore file > env > defaults. Measured:

keyin config/uplink.ts?UPLINK_* setresult
pollMsyes551234, the file wins
workdirno/from/env/from/env

Every key listed in config/uplink.ts would have stopped responding to its variable. No error, no log. Since .env is how Uplink is configured and uplink:install writes one, that precedence is backwards for this app.

The fix keeps bunfig as the single mechanism but calls its exported synchronous applyEnvVarsToConfig directly, so the file is the defaults and the environment wins. tests/unit/uplink/config.test.ts pins it, because the failure mode is silent.

One consequence worth recording: bunfig's index does not tree-shake, so the bundled desktop entry went from 101.90 KB to 400 KB. Under 1% of the DMG, and bunfig/browser is 8 KB but does not export the function.

Auto-imports: keep the explicit imports, and here is the proof

The trap this issue describes is real, and it decides the question rather than complicating it.

injectGlobalAutoImports() is called from exactly six places in the installed packages:

  • @stacksjs/buddy/dist/production-server.js
  • @stacksjs/buddy/dist/commands/seed.js
  • @stacksjs/actions/dist/dev/views.js, dev/api.js, dev/dashboard.js, serve/api.js

Every one is a server or dev-server boot path. buddy/dist/cli.js and custom-cli.js, which are what run a file in app/Commands/, do not mention it at all, and neither does anything Uplink owns.

Uplink's engine runs in two processes, and neither is on that list:

  1. buddy uplink:watch, a plain custom command under launchd.
  2. Uplink.app, a bun build --compile binary with no framework in it at all.

So moving codex-engine.ts to resources/functions/ and calling new CodexEngine(...) bare would typecheck, regenerate cleanly, appear in server-auto-imports.d.ts, and throw ReferenceError on the first text anyone sends. That is stacksjs/stacks#2585 with a different symbol.

So the engine modules keep their explicit imports. That is not a workaround: it is what the framework's own code does. The built-in actions import Action, route, response and schema from their packages even though all four are declared as globals, and AGENTS.md says to copy that pattern.

Uplink has no code that runs inside the server, so there is currently nothing for resources/functions/ to hold. If a dashboard action arrives later, that is where its helpers belong.

The specific suggestion in the split above, codex-engine.ts, engine.ts, format.ts and filter.ts moving to resources/functions/, is the one that would break, and messages-db.ts / typedstream.ts / sqlite-store.ts staying put is right for the reason given.

Written down so it does not need rediscovering

Both traps are now in app/Skills/uplink-conventions/SKILL.md (#41), with the verification recipe: regenerate, confirm the declaration, then log typeof globalThis.X in the process that actually uses it. Steps one and two pass in the broken case, which is the whole problem.

Remaining on this issue: nothing, unless the "no code runs in the server" premise changes.

Sign in to comment on this issue.