-
Notifications
You must be signed in to change notification settings - Fork 428
refactor: better configuration #2854
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
manucorporat
wants to merge
1
commit into
main
Choose a base branch
from
refactor-configuration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| --- | ||
| name: configuration | ||
| description: >- | ||
| Where a configuration value belongs — app config schema, agent-native.config.ts, | ||
| or an environment variable — and how the layers resolve. Use before adding an | ||
| env var, a configure*/set* function, a plugin option, or a register* function, | ||
| and when deciding whether something is config or a registry. | ||
| scope: dev | ||
| metadata: | ||
| internal: true | ||
| --- | ||
|
|
||
| # Configuration | ||
|
|
||
| Core had 301 distinct environment variables and only 48 of them were secrets. | ||
| The other 253 are product decisions that landed in `process.env` because at the | ||
| call site that needed them, nothing else was reachable. This skill exists so the | ||
| next value does not do the same thing. | ||
|
|
||
| ## The rule | ||
|
|
||
| **Consumer code never reads `process.env`.** Exactly four resolvers do: | ||
|
|
||
| | Resolver | Covers | | ||
| | --- | --- | | ||
| | The app config env layer (`app-config/env-layer.ts`) | product behavior, via a declared `.meta({ env })` alias | | ||
| | `resolveDeployEnvironment()` | platform facts — `NODE_ENV`, `NETLIFY`, `AWS_*` | | ||
| | `readDeployCredentialEnv()` | secrets, as the deployment layer inside scoped resolution | | ||
| | `getAmbientUserEmail()` / `getAmbientOrgId()` | CLI identity when there is no request context | | ||
|
|
||
| The bottom three are not configuration: platform variables are facts nobody | ||
| sets in an app, credentials resolve per user or org, and ambient identity exists | ||
| only for CLI runs. Everything else is a field in the schema. | ||
|
|
||
| This is the credential rule ("one resolver per key, and every runtime path goes | ||
| through it") applied to all configuration, with a schema lookup replacing the | ||
| grep. | ||
|
|
||
| ## Where a value goes | ||
|
|
||
| | Value | Where | | ||
| | --- | --- | | ||
| | Server behavior, closures, per-tenant resolvers, anything not for a browser | `defineAppConfig()` | | ||
| | Client-visible data | `agent-native.config.ts` | | ||
| | A deployment override of a server value | an `env` alias on the schema field | | ||
| | A secret value | the vault, via `resolveCredential` — see the `secrets` skill | | ||
| | Something a user changes at runtime in the UI | the settings store, not config | | ||
|
|
||
| `agent-native.config.ts` is serialized into the bundle and hard-cached in a | ||
| public SSR shell. Moving a value there to avoid threading it publishes a | ||
| deployment fact to every visitor. It also cannot carry a closure. | ||
|
|
||
| ## Adding a field | ||
|
|
||
| Add it to the domain file under `packages/core/src/app-config/`, or add a new | ||
| domain file plus one line in `schema.ts`. | ||
|
|
||
| ```ts | ||
| // packages/core/src/app-config/email.ts | ||
| export const emailConfig = z.object({ | ||
| brandColor: z.string().regex(HEX_COLOR).optional().meta({ | ||
| env: "EMAIL_BRAND_COLOR", | ||
| doc: "Accent color for framework-rendered emails.", | ||
| }), | ||
| renderer: z.custom<EmailRenderer>().optional(), | ||
| }); | ||
| ``` | ||
|
|
||
| Then read it: `getAppConfig().email.brandColor`. | ||
|
|
||
| Four things worth knowing before you write one: | ||
|
|
||
| - **Wrap a domain in `.prefault({})`, never `.optional()` or `.default({})`.** | ||
| An optional domain never materializes the defaults declared inside it, and | ||
| `.default({})` hands back the literal `{}` without parsing it. Both leave a | ||
| reader with `undefined` where the type promises a value. | ||
| - **`.meta({ env })` belongs on leaf fields only.** Collection throws on a group | ||
| that declares one, because the alias would silently never fire. | ||
| - **A closure is an ordinary field.** `z.custom<Fn>()` survives `.parse()`, so a | ||
| renderer or a per-org resolver does not need a separate mechanism. | ||
| - **An env alias needs a parser for its type.** Strings, enums, booleans, and | ||
| numbers are handled; anything else throws at startup rather than injecting a | ||
| string into a field that cannot hold one. | ||
| - **`env` can be an ordered list**, and that is how one concept with many | ||
| historical spellings collapses: | ||
| `.meta({ env: ["AGENT_NATIVE_APP_ID", "APP_ID"] })`. First key that is set | ||
| wins. A key that is unset, empty, or whitespace counts as absent, matching the | ||
| `?.trim() ||` every hand-rolled chain used. | ||
|
|
||
| Adding a spelling to an existing field's alias list **widens every reader of | ||
| that field**, not just the one you are looking at. App identity is the worked | ||
| example: `credential-provider` had no `APP_ID` in its chain, so adding it there | ||
| changed which id scopes a credential grant. That can be the right call, but it | ||
| is a decision to make deliberately, not a tidy-up. | ||
|
|
||
| ## Resolution order | ||
|
|
||
| Lowest opinion first: **declared default → env → deprecated `set*` setter → | ||
| `defineAppConfig()`**. | ||
|
|
||
| Env sits below app code, which inverts twelve-factor on purpose: a typed, | ||
| reviewed, checked-in value should beat an ambient string on the host. It is safe | ||
| because nothing above env sets these keys yet, so env still wins every lookup | ||
| until someone deliberately adds a layer above it. | ||
|
|
||
| Values are validated where they are set, so a bad value names the call site that | ||
| set it rather than whichever unrelated read ran first. | ||
|
|
||
| ## Config or registry? | ||
|
|
||
| The distinguishing property is the merge rule, not the number of entries. | ||
|
|
||
| > Config layers **override** — the highest layer wins. | ||
| > Registries **accumulate** — every source's entries coexist. | ||
|
|
||
| If a module has `getActive*()` or a "first wins" rule, it is config wearing a | ||
| registry's name. If it has `list*()` and everything fires, or `get(id)`, it is a | ||
| registry and it stays one. | ||
|
|
||
| There is a third shape that is easy to get wrong in both directions: | ||
| accumulate-but-only-one-is-used. Private blob and file upload providers are | ||
| this. The registry is correct — apps really do add providers — and the defect is | ||
| only that nothing states which one is active. The fix is additive: keep the | ||
| registry, add a selector field, keep the old first-configured rule as the | ||
| fallback so no existing deployment changes behavior. | ||
|
|
||
| `defineTransactionalEmail` is the clearest case of a real registry: core | ||
| registers its system emails and each app registers its own. Model that as an | ||
| array setting and the app's value replaces core's, so password reset silently | ||
| disappears from the catalog. | ||
|
|
||
| ## Antipatterns | ||
|
|
||
| - **Reading `process.env` outside the four resolvers.** Add the field, give it | ||
| an `env` alias. | ||
| - **A bespoke `configure*` / `set*` function for one domain.** That is a second | ||
| namespace with no precedence story and no discoverability. Add a field. | ||
| - **A `register*` for something with exactly one active instance.** Apply the | ||
| merge-rule test. | ||
| - **A plugin option that duplicates an env var.** The ladder already makes one | ||
| field reachable from both. `createAgentChatPlugin({ model })` keeps working — | ||
| the option becomes the top layer of that field rather than a private closure | ||
| value. | ||
| - **Moving a value to `agent-native.config.ts` to avoid threading it.** | ||
|
|
||
| ## Deprecated paths | ||
|
|
||
| Deprecate, do not delete — core is published, so removal waits for a major and | ||
| that is how this gets deferred forever. Mark the old export `@deprecated` with | ||
| "Use `X` instead", point it at the `legacy` config layer so it keeps working, | ||
| and add a row to the register in | ||
| `plans/core-configuration-attack-plan.md`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --- | ||
| "@agent-native/core": minor | ||
| --- | ||
|
|
||
| Add `defineAppConfig()` and `getAppConfig()` — one zod schema under `src/app-config/` that owns server-side configuration, so a value can be set in typed app code instead of only through an environment variable. Environment variables become declared `.meta({ env })` aliases into a schema field, parsed and validated in one place rather than at each call site, and resolve below explicit app configuration. A field can declare several aliases in precedence order, which is how one concept with many historical spellings collapses to a single declared ladder. | ||
|
|
||
| Five domains are declared so far, replacing roughly thirty hand-rolled `process.env` reads: | ||
|
|
||
| - **`privateBlob`** — `provider` selects which registered provider is active, replacing the implicit "first one whose `isConfigured()` returns true in module import order" rule (still the fallback when unset), and throwing when the named provider is not registered. `publicUploadFallback` replaces a setter and an environment variable whose precedence was decided by statement order inside `putPrivateBlob`. `setPrivateBlobPublicUploadFallbackEnabled` is deprecated but keeps working, now with a stated position in the ladder. | ||
| - **`app`** — `id`, `workspaceId`, `name`, `packageName`, and `template` replace nine fallback chains across agent chat, SSO, credential scoping, onboarding, the CLI, data programs, durable background dispatch, and workspace OAuth. They stay separate fields on purpose: `vault_grants` rows are written with the workspace-assigned id, so credential scoping keeps preferring it, and `name` is a display name rather than an identifier. None has a default, so an app with no configured identity is still denied a credential grant lookup instead of resolving one scoped to an app literally named `app`. | ||
| - **`agent`** — `engine`, `model`, `mode`, `preferBringYourOwnKey`, `runSoftTimeoutMs`, `completedRunRetentionMs`, `erroredRunRetentionMs`. `resolveEngine`'s documented resolution order is unchanged and `createAgentChatPlugin({ model })` keeps working; the explicit option stays a function parameter above the declared field. | ||
| - **`a2a`** and **`integrations`** — `allowUnsignedInternal` and `allowUnverifiedWebhooks`, the latter replacing three byte-identical copies of the same check in the telegram, whatsapp, and email webhook adapters. | ||
|
|
||
| - **`workspace`** — `gatewayUrl` and `oauthOrigin`. Together with `app.url` these retire the `VITE_` mirrors of the URL keys: the prefix only ever answered "how does this value reach the browser", so the value is now one declared field and delivery goes through `window.__AGENT_NATIVE_CONFIG__` alongside the existing Sentry, PostHog, and realtime scripts. | ||
|
|
||
| Two self-dispatch bugs are fixed along the way. `integrations/webhook-handler.ts` and `integrations/a2a-continuation-processor.ts` each carried their own copy of "resolve my own base URL"; both omitted `DEPLOY_PRIME_URL`, so a Netlify deploy preview dispatched background work to production, and the continuation copy silently fell back to `http://localhost:${PORT}` in production, where the request never arrives and the work is dropped with no error. Both now delegate to `resolveSelfDispatchBaseUrl`. | ||
|
|
||
| A new guard keeps the surface from growing back: `pnpm guard:no-legacy-config` fails when a line this branch adds reads `process.env` in `packages/core/src` outside the four resolvers, or calls a deprecated entry point. Opt out per line with `// config-ok: <reason>`. | ||
|
|
||
| Declared configuration now generates its own documentation: `pnpm sync:config-docs` writes the field table into `docs/environment-variables.md`, and `pnpm guard:config-docs` fails when it is stale. | ||
|
|
||
| Malformed values in migrated keys now fail at startup naming the key, instead of silently reading as `false` or falling back to a default. This affects `AGENT_ENGINE_PREFER_BYO_KEY`, `A2A_ALLOW_UNSIGNED_INTERNAL`, `AGENT_NATIVE_ALLOW_UNVERIFIED_WEBHOOKS`, and the three agent run timeout/retention keys. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Regenerate the declared configuration table
pnpm guard:config-docscurrently fails because the checked-in generated table does not exactly matchscripts/sync-config-docs.tsoutput (the committed table is padded while the generator emits unpadded rows). Since this guard is registered in the guard suite, the branch cannot pass from a clean checkout; regenerate the block and commit the result.Additional Info