From 175bebcdecf7c82eba4321318f9ba479339e2ac4 Mon Sep 17 00:00:00 2001 From: Thada Wangthammang Date: Mon, 3 Aug 2026 12:11:14 +0000 Subject: [PATCH] feat(milestone-2/batch-1.5): realign EnvConnector with synthing's architecture Batch 1 shipped EnvConnector by copying kubricate's implementation mostly unchanged, but its own task-5 spec (uppercase transform) never matched the shipped code (exact-case matching), and it carried over kubricate mechanisms that don't fit synthing's design: SecretValue (shaped by BaseProvider's Kubernetes-Secret serialization constraint, which synthing has no equivalent of) and tryParseSecretValue() (connector-side JSON-sniffing that collides with the engine's coerceFromString(), since a connector never knows a key's declared type). A grilling session (design-spec.md decisions 94-99) worked through the realignment; this closes it out before Batch 2's resolution engine builds on the coercion boundary. - BaseConnector: abstract class -> interface (structural typing avoids the dual-package hazard for connector plugins); config/logger/ setWorkingDir?/getWorkingDir? join load/get as contract members - EnvConnector: drop SecretValue + tryParseSecretValue, get() returns raw string only; caseInsensitive defaults to true; new maskValues config (default true) masks all logged values indiscriminately, since the connector has no visibility into which keys are secrets - Add .chief/milestone-2/_contract/base-connector-contract.md as the durable record of this service boundary Also fixes three pre-existing scaffolding gaps uncovered along the way: toolkit, plugin-env, and synthing were all missing eslint.config.mjs (lint had never actually run on them), and coerce.ts had an unused catch binding. Co-Authored-By: Claude Sonnet 5 --- .../_contract/base-connector-contract.md | 49 ++++++++++ .chief/milestone-2/_goal/design-spec.md | 41 ++++++-- .chief/milestone-2/_plan/_todo.md | 27 +++-- .chief/milestone-2/_plan/task-6.md | 51 ++++++++++ .chief/milestone-2/_plan/task-7.md | 57 +++++++++++ .chief/milestone-2/_plan/task-8.md | 45 +++++++++ .chief/milestone-2/_plan/task-9.md | 54 ++++++++++ packages/core/src/connector.ts | 26 ++++- packages/core/src/index.ts | 2 +- packages/plugin-env/eslint.config.mjs | 14 +++ packages/plugin-env/src/env-connector.test.ts | 96 +++++++++++------- packages/plugin-env/src/env-connector.ts | 98 ++++++++----------- packages/plugin-env/src/index.ts | 3 +- packages/plugin-env/src/utils.ts | 14 +++ packages/synthing/eslint.config.mjs | 4 + packages/toolkit/eslint.config.mjs | 4 + packages/toolkit/src/coerce.ts | 2 +- 17 files changed, 471 insertions(+), 116 deletions(-) create mode 100644 .chief/milestone-2/_contract/base-connector-contract.md create mode 100644 .chief/milestone-2/_plan/task-6.md create mode 100644 .chief/milestone-2/_plan/task-7.md create mode 100644 .chief/milestone-2/_plan/task-8.md create mode 100644 .chief/milestone-2/_plan/task-9.md create mode 100644 packages/plugin-env/eslint.config.mjs create mode 100644 packages/plugin-env/src/utils.ts create mode 100644 packages/synthing/eslint.config.mjs create mode 100644 packages/toolkit/eslint.config.mjs diff --git a/.chief/milestone-2/_contract/base-connector-contract.md b/.chief/milestone-2/_contract/base-connector-contract.md new file mode 100644 index 0000000..1a88b66 --- /dev/null +++ b/.chief/milestone-2/_contract/base-connector-contract.md @@ -0,0 +1,49 @@ +# Contract: BaseConnector + +Service boundary between `@synthing/core` and any connector plugin (e.g. `@synthing/plugin-env`). Locked by the grilling session recorded in `.chief/milestone-2/_goal/design-spec.md` §14 (decisions 94–99). Must not be violated without a new grilling session updating both this file and the design-spec. + +## Interface shape + +```ts +// @synthing/core +interface BaseConnector { + config: Config; + logger?: Logger; + + load(keys: string[]): Promise; + get(key: string): unknown | undefined; + + setWorkingDir?(dir: string | undefined): void; + getWorkingDir?(): string | undefined; +} +``` + +- **`interface`, not `abstract class`.** Structural typing — a connector plugin's installed `@synthing/core` copy does not need to be the same class instance as the host's (avoids the dual-package hazard). +- `config`, `logger`, `setWorkingDir?`, `getWorkingDir?` are part of the contract, not connector-specific extras. Engine code may call `connector.setWorkingDir?.(dir)` on any `BaseConnector` without narrowing to a concrete class. + +## Coercion boundary (hard rule) + +**Connectors never coerce, parse, or transform values.** `get()` returns the raw value exactly as read from the source. + +Rationale: `load(keys: string[])` only receives key *names* — a connector has no way to know a key's declared `VariableType`. Any connector-side type inference is necessarily a blind guess based on value shape, not the user's declared intent, and can double-coerce a value the engine's `coerceFromString()` then tries to coerce again (e.g. a connector that JSON-parses a flat-object-looking string before the engine's `coerceFromString(value, "object")` — which expects a raw string — ever sees it). + +`@synthing/toolkit.coerceFromString(rawValue, declaredType)` is the **only** coercion path, called exclusively by the engine, after `get()` returns. + +## No `SecretValue` type + +Kubricate's `SecretValue` (`string | number | boolean | null | undefined | Record`) existed to satisfy `BaseProvider.prepare(name, value: SecretValue)` — Kubernetes Secrets are flat, string-serializable key-value maps, so the type was shaped by that downstream serialization constraint. + +Synthing has no `BaseProvider` equivalent. Nothing downstream constrains connector output shape. `get()` stays `unknown | undefined` at the interface level; concrete connectors narrow it to whatever their source actually produces (`EnvConnector` → `string | undefined`, since env vars are always strings). + +## Logging (connector-specific, not part of the core interface) + +A connector MAY log values it handles, but must not assume it knows which keys are secrets — `secret: true` is a `VariableManager`-level declaration the connector never sees. `EnvConnector` masks every logged value indiscriminately by default (`maskValues: true`) rather than guessing. + +## Concrete instance: EnvConnector + +| Aspect | Contract | +|---|---| +| `get()` return type | `string \| undefined` (no `SecretValue`, no object coercion) | +| Key matching | `expectedKey = prefix + key`, matched against `process.env` case-insensitively by default (`caseInsensitive: true`) | +| Value transformation | None — raw string only, no JSON-sniffing | +| Logging | `maskValues: true` by default — all logged values masked via `maskingValue()` | diff --git a/.chief/milestone-2/_goal/design-spec.md b/.chief/milestone-2/_goal/design-spec.md index d6bf9d5..6c0a162 100644 --- a/.chief/milestone-2/_goal/design-spec.md +++ b/.chief/milestone-2/_goal/design-spec.md @@ -121,7 +121,9 @@ For a given key, the value precedence is: ### 2.6 Type Coercion -**Shared coercion utilities (from `@synthing/toolkit`)** — toolkit exports `coerceFromString()` helpers. Connectors and the engine may use them. The engine performs the final type-check to ensure basic type matching (a variable declared as `number` returns a number or errors). +**Shared coercion utilities (from `@synthing/toolkit`)** — toolkit exports `coerceFromString()` helpers. **Coercion is engine-owned only.** Connectors return raw values exactly as received from their source (e.g. `EnvConnector` returns the raw `process.env` string, untouched) — they never parse, transform, or guess types. The engine is the sole caller of `coerceFromString()`, using the `type` declared via `.addVariable()`, and performs the final type-check (a variable declared as `number` returns a number or errors). + +> A connector never knows a key's declared `type` — `load(keys: string[])` only receives key names. Any connector-side coercion would be a blind guess based on value shape, not the user's declared intent, and risks double-coercing values the engine then tries to coerce again. See `.chief/milestone-2/_contract/base-connector-contract.md` for the full rationale. ```ts // @synthing/toolkit exports @@ -239,26 +241,36 @@ Users declare keys via `.addVariable()`. Connectors do not contribute or discove ### 4.1 BaseConnector (in `@synthing/core`) -`BaseConnector` is **domain-agnostic**. The same connector class serves both variables and secrets. It lives in `@synthing/core` as a pure abstract interface. +`BaseConnector` is **domain-agnostic**. The same connector implementation serves both variables and secrets. It lives in `@synthing/core` as a TypeScript **`interface`** (not an `abstract class`) — structural typing avoids the dual-package hazard for third-party connector plugins (a plugin's installed `@synthing/core` copy doesn't need to be the exact same class instance as the host's; it only needs to match the shape). Naming: `BaseConnector`, not `BaseVariableConnector` or `BaseVariableResolver`. Aligns with kubricate's `BaseConnector` term. +`BaseConnector` has no `SecretValue`-style return type. Kubricate's `SecretValue` union existed to satisfy `BaseProvider.prepare()`'s requirement that secrets be flat and string-serializable for Kubernetes Secret encoding — synthing has no `BaseProvider` equivalent, so there's nothing downstream constraining connector output shape. `get()` returns `unknown | undefined`; concrete connectors narrow it (e.g. `EnvConnector` narrows to `string | undefined`, since env vars are always strings). + ### 4.2 Two-Phase Interface -Matches kubricate's `BaseConnector` interface: +Matches kubricate's `BaseConnector` interface shape, including the optional working-dir and logger members: ```ts -abstract class BaseConnector { +interface BaseConnector { + config: Config; + logger?: Logger; + /** Load/prepare values for the given keys (async, called once) */ - abstract load(keys: string[]): Promise; + load(keys: string[]): Promise; /** Get a single value (sync, called per-key after load) */ - abstract get(key: string): unknown | undefined; + get(key: string): unknown | undefined; + + /** Optional — no-op if a connector doesn't need a working directory */ + setWorkingDir?(dir: string | undefined): void; + getWorkingDir?(): string | undefined; } ``` - `load(keys[])` — called once with all keys this connector might need. Allows batch fetching. - `get(key)` — called per-key after load. Returns the raw value or `undefined`. +- `config`, `logger`, `setWorkingDir?`, `getWorkingDir?` are part of the shared contract (not connector-specific extras), so engine code can address any connector generically without narrowing to a concrete class. ### 4.3 EnvConnector (in `@synthing/plugin-env`) @@ -275,6 +287,12 @@ new EnvConnector({ prefix: "APP_" }) Users can separate variable vs secret env vars via prefix convention (e.g., `VAR_`, `SECRET_`). +**Env var name matching:** `expectedKey = prefix + key`, matched against `process.env` **case-insensitively by default** (`caseInsensitive` defaults to `true`). A declared key `port` with prefix `APP_` matches `APP_PORT`, `APP_port`, or any other casing — there is no uppercase transform of the key itself, matching is case-agnostic instead. This is why the example above (`APP_PORT`) and a lowercase-declared key both work without any extra configuration. + +**Coercion boundary:** `get()` returns the raw `process.env` string exactly as read — never JSON-parsed, never transformed. Kubricate's `tryParseSecretValue()` (best-effort JSON-sniffing) is intentionally **not** carried over; per 2.6, coercion is engine-owned only. + +**Logging:** `maskValues` config, defaults to `true`. Every value the connector logs is masked via a `maskingValue()` utility, regardless of whether the key is a declared secret — the connector has no visibility into `secret: true` (that's a `VariableManager`-level fact), so it masks indiscriminately rather than guessing. Set `maskValues: false` to log raw values (local debugging only). + --- ## 5. Generator System @@ -1118,3 +1136,14 @@ For traceability, each major decision is numbered. These numbers correspond to t 91. `BaseGenerator.format` is `string` (not literal union) — extensible, engine does map lookup 92. CLI uses `yargs` (same as kubricate) 93. Config loading uses `unconfig` (same as kubricate) + +--- + +### Grilling session 2 — EnvConnector realignment (post-Batch-1) + +94. `BaseConnector` is a TypeScript `interface`, not an `abstract class` — structural typing avoids the dual-package hazard for third-party connector plugins +95. `BaseConnector` interface includes `config`, `logger`, `setWorkingDir?`, `getWorkingDir?` as contract members, not just `load`/`get` — engine code can address any connector generically +96. No connector-side coercion — `load(keys: string[])` only receives key names, never declared types, so connectors cannot do type-aware coercion. `@synthing/toolkit.coerceFromString` at the engine layer is the sole coercion path +97. `SecretValue` type dropped entirely — it was a kubricate artifact tied to `BaseProvider`'s Kubernetes-Secret string-serialization constraint, which synthing has no equivalent of. `EnvConnector.get()` narrows to `string | undefined` +98. `EnvConnector.caseInsensitive` defaults to `true` — env var name matching is case-agnostic by default, so declared lowercase keys match uppercase env vars (and vice versa) without extra config +99. `EnvConnector` adds `maskValues` config, default `true` — every logged value is masked via `maskingValue()` regardless of secret-ness, since the connector has no visibility into which keys are declared `secret: true` diff --git a/.chief/milestone-2/_plan/_todo.md b/.chief/milestone-2/_plan/_todo.md index 2ad785f..4b965fa 100644 --- a/.chief/milestone-2/_plan/_todo.md +++ b/.chief/milestone-2/_plan/_todo.md @@ -8,17 +8,26 @@ - [x] task-4: Implement VariableManager with builder pattern, $var branded refs, and $spread helper - [x] task-5: Implement EnvConnector in @synthing/plugin-env +## Batch 1.5: EnvConnector Realignment (blocks Batch 2 — resolution engine depends on the coercion boundary this batch locks in) + +Handoff from the grilling session recorded in `.chief/milestone-2/_goal/design-spec.md` §14 (decisions 94–99) and `.chief/milestone-2/_contract/base-connector-contract.md`. Batch 1 shipped `EnvConnector` with behavior that drifted from its own task spec (task-5.md said "uppercase transform"; the shipped code does exact-case matching) and carried over kubricate mechanisms (`SecretValue`, `tryParseSecretValue`) that don't fit synthing's architecture. This batch reconciles code, contract, and docs before Batch 2 builds on top of it. + +- [x] task-6: Convert `BaseConnector` from `abstract class` to `interface`; add `config`/`logger`/`setWorkingDir?`/`getWorkingDir?` to the contract +- [x] task-7: Remove `SecretValue` and `tryParseSecretValue()` from `EnvConnector`; narrow `get()` to `string | undefined` +- [x] task-8: Default `caseInsensitive` to `true` in `EnvConnectorConfig`; add a test locking in default uppercase-env-var matching +- [x] task-9: Add `maskValues` config (default `true`); restore a `maskingValue()` utility; mask all logged values by default + ## Batch 2: Engine, Generators, and Pipeline (planned, not yet detailed) -- [ ] task-6: Implement resolution engine (tag scanning, resolve, resolveAll, strict/loose mode) -- [ ] task-7: Implement YamlGenerator and GeneratorContext -- [ ] task-8: Implement generator pipeline runner -- [ ] task-9: Implement text pipeline (plain text and structural modes) -- [ ] task-10: Implement $spread resolution in structural mode +- [ ] task-10: Implement resolution engine (tag scanning, resolve, resolveAll, strict/loose mode) +- [ ] task-11: Implement YamlGenerator and GeneratorContext +- [ ] task-12: Implement generator pipeline runner +- [ ] task-13: Implement text pipeline (plain text and structural modes) +- [ ] task-14: Implement $spread resolution in structural mode ## Batch 3: CLI, Config, Integration, and Acceptance (planned, not yet detailed) -- [ ] task-11: Implement defineConfig() and config loading -- [ ] task-12: Implement CLI (synthing generate, synthing variable export-schema) -- [ ] task-13: Write integration tests and kubricate workflow tests -- [ ] task-14: Full acceptance verification against all criteria +- [ ] task-15: Implement defineConfig() and config loading +- [ ] task-16: Implement CLI (synthing generate, synthing variable export-schema) +- [ ] task-17: Write integration tests and kubricate workflow tests +- [ ] task-18: Full acceptance verification against all criteria diff --git a/.chief/milestone-2/_plan/task-6.md b/.chief/milestone-2/_plan/task-6.md new file mode 100644 index 0000000..47038ba --- /dev/null +++ b/.chief/milestone-2/_plan/task-6.md @@ -0,0 +1,51 @@ +# Task 6: Convert BaseConnector from abstract class to interface + +## Objective + +Convert `BaseConnector` in `@synthing/core` from an `abstract class` to a TypeScript `interface`, and add `config`, `logger`, `setWorkingDir?`, `getWorkingDir?` as part of the contract. Update `EnvConnector` to `implements` the interface instead of `extends` the class. + +## Scope + +### Included +- `packages/core/src/connector.ts`: replace `abstract class BaseConnector` with `interface BaseConnector` +- Add `config: Config`, `logger?: Logger`, `setWorkingDir?(dir: string | undefined): void`, `getWorkingDir?(): string | undefined` to the interface +- `packages/plugin-env/src/env-connector.ts`: change `class EnvConnector extends BaseConnector` to `class EnvConnector implements BaseConnector` +- Remove `super()` call and `override` keyword from `EnvConnector` (no longer meaningful against an interface) + +### Excluded +- `SecretValue` / `tryParseSecretValue()` removal (task-7) +- `caseInsensitive` default change (task-8) +- `maskValues` / logging changes (task-9) + +## Rules & Contracts to Follow +- `.chief/_rules/_standard/coding-standards.md` +- `.chief/milestone-2/_goal/design-spec.md` Sections 4.1, 4.2 +- `.chief/milestone-2/_contract/base-connector-contract.md` + +## Steps + +1. Edit `packages/core/src/connector.ts`: interface definition per the contract doc +2. Edit `packages/plugin-env/src/env-connector.ts`: `implements` instead of `extends`, drop `super()`/`override` +3. Run `pnpm check-types` and fix any fallout +4. Run `pnpm test` to confirm no behavioral regressions + +## Acceptance Criteria + +- `BaseConnector` is declared as an `interface`, not a `class`, in `@synthing/core` +- `EnvConnector implements BaseConnector` +- `pnpm build` exits 0 +- `pnpm check-types` exits 0 +- `pnpm test` exits 0 + +## Verification + +```bash +pnpm build +pnpm check-types +pnpm test --filter @synthing/core --filter @synthing/plugin-env +``` + +## Deliverables + +- `packages/core/src/connector.ts` (updated) +- `packages/plugin-env/src/env-connector.ts` (updated) diff --git a/.chief/milestone-2/_plan/task-7.md b/.chief/milestone-2/_plan/task-7.md new file mode 100644 index 0000000..32cfac8 --- /dev/null +++ b/.chief/milestone-2/_plan/task-7.md @@ -0,0 +1,57 @@ +# Task 7: Remove SecretValue and tryParseSecretValue from EnvConnector + +## Objective + +Remove the `SecretValue` type and `tryParseSecretValue()` method from `EnvConnector`. The connector must return raw `process.env` string values only — no JSON-sniffing, no coercion. This closes the coercion-boundary gap: `tryParseSecretValue()` could hand the engine an already-parsed object where `coerceFromString()` expects a raw string, breaking exactly the `type: "object"` case it's meant to handle. + +## Scope + +### Included +- Delete `tryParseSecretValue()` method from `packages/plugin-env/src/env-connector.ts` +- Delete the `SecretValue` type definition and its export from `packages/plugin-env/src/index.ts` +- `get(key: string)` return type narrows to `string | undefined` +- `load()` stores the raw string directly (no `tryParseSecretValue()` call) +- Remove the `tryParseSecretValue()` describe block from `env-connector.test.ts`; remove any test asserting object-parsing behavior + +### Excluded +- `BaseConnector` interface conversion (task-6, should land first) +- `caseInsensitive` / `maskValues` changes (tasks 8, 9) + +## Rules & Contracts to Follow +- `.chief/_rules/_standard/coding-standards.md` +- `.chief/milestone-2/_goal/design-spec.md` Section 2.6, Section 4.3 ("Coercion boundary") +- `.chief/milestone-2/_contract/base-connector-contract.md` + +## Steps + +1. Remove `tryParseSecretValue()` from `env-connector.ts` +2. Remove `SecretValue` type and its export from `index.ts` +3. Update `get()` signature and `this.secrets` map type to `Map` +4. Update `load()` to store `process.env[matchKey]` directly, no parsing +5. Remove/update affected tests in `env-connector.test.ts` +6. Confirm no other package imports `SecretValue` from `@synthing/plugin-env` + +## Acceptance Criteria + +- No `SecretValue` type or export anywhere in `@synthing/plugin-env` +- No `tryParseSecretValue` method anywhere in the codebase +- `EnvConnector.get()` is typed `string | undefined` +- A flat-JSON-looking env var value (e.g. `'{"a":1}'`) is returned as the raw string `'{"a":1}'`, unparsed +- `pnpm build` exits 0 +- `pnpm check-types` exits 0 +- `pnpm test` exits 0 + +## Verification + +```bash +pnpm build +pnpm check-types +pnpm test --filter @synthing/plugin-env +grep -rn "SecretValue\|tryParseSecretValue" packages/ # should return nothing +``` + +## Deliverables + +- `packages/plugin-env/src/env-connector.ts` (updated) +- `packages/plugin-env/src/index.ts` (updated) +- `packages/plugin-env/src/env-connector.test.ts` (updated) diff --git a/.chief/milestone-2/_plan/task-8.md b/.chief/milestone-2/_plan/task-8.md new file mode 100644 index 0000000..0d9a767 --- /dev/null +++ b/.chief/milestone-2/_plan/task-8.md @@ -0,0 +1,45 @@ +# Task 8: Default caseInsensitive to true + +## Objective + +Change `EnvConnectorConfig.caseInsensitive` default from `false` to `true`. This resolves the mismatch between the design-spec's CLI examples (which use uppercase env vars like `APP_PORT`) and the connector's exact-case-key lookup (`prefix + key`, e.g. `APP_port` for key `port`) — with case-insensitive matching on by default, both forms resolve to the same variable without any doc or code disagreement. + +## Scope + +### Included +- `packages/plugin-env/src/env-connector.ts`: change `this.caseInsensitive = config.caseInsensitive ?? false` to `?? true` +- Add a test confirming default (no explicit `caseInsensitive` flag) matches an uppercase env var against a lowercase-declared key +- Confirm existing exact-case tests still pass (case-insensitive matching is a superset of exact-case matching) + +### Excluded +- `BaseConnector` interface conversion (task-6) +- `SecretValue` / coercion removal (task-7) +- `maskValues` logging (task-9) + +## Rules & Contracts to Follow +- `.chief/_rules/_standard/coding-standards.md` +- `.chief/milestone-2/_goal/design-spec.md` Section 4.3 ("Env var name matching") +- `.chief/milestone-2/_contract/base-connector-contract.md` + +## Steps + +1. Change the default in the `EnvConnector` constructor +2. Add test: `new EnvConnector({ prefix: "APP_" })` (no `caseInsensitive` specified), `process.env.APP_PORT = "8080"`, `load(["port"])`, `get("port")` returns `"8080"` +3. Re-run the full `env-connector.test.ts` suite to confirm no existing test relied on strict-by-default behavior + +## Acceptance Criteria + +- `new EnvConnector({ prefix: "APP_" })` with `process.env.APP_PORT` set matches key `port` without `caseInsensitive: true` being passed explicitly +- Existing exact-case tests (e.g. `APP_port` matching key `port`) still pass unchanged +- `pnpm test` exits 0 + +## Verification + +```bash +pnpm test --filter @synthing/plugin-env +``` + +## Deliverables + +- `packages/plugin-env/src/env-connector.ts` (updated) +- `packages/plugin-env/src/env-connector.test.ts` (updated) diff --git a/.chief/milestone-2/_plan/task-9.md b/.chief/milestone-2/_plan/task-9.md new file mode 100644 index 0000000..b583575 --- /dev/null +++ b/.chief/milestone-2/_plan/task-9.md @@ -0,0 +1,54 @@ +# Task 9: Add maskValues config and restore maskingValue() utility + +## Objective + +Add a `maskValues?: boolean` config to `EnvConnectorConfig`, defaulting to `true`. Restore a `maskingValue()` utility (ported from kubricate) and log every value the connector handles through it by default — regardless of whether the key is a declared secret, since the connector has no visibility into `secret: true` (a `VariableManager`-level fact). `maskValues: false` opts into raw-value logs. + +## Scope + +### Included +- `packages/plugin-env/src/utils.ts` (new): `maskingValue(value: string, length = 4): string` — same behavior as kubricate's version (first `length` chars kept, rest replaced with `*`) +- `packages/plugin-env/src/env-connector.ts`: add `maskValues?: boolean` to `EnvConnectorConfig`, default `true` in constructor +- In `load()`, after resolving each value, log it via the connector's `logger` — masked through `maskingValue()` when `maskValues` is `true` (default), raw when `false` +- Tests: default masks the logged value; `maskValues: false` logs the raw value + +### Excluded +- `BaseConnector` interface conversion (task-6) +- `SecretValue` / coercion removal (task-7) — by the time this task lands, `get()` already returns raw strings only, so masking only ever operates on strings +- `caseInsensitive` default (task-8) + +## Rules & Contracts to Follow +- `.chief/_rules/_standard/coding-standards.md` +- `.chief/milestone-2/_goal/design-spec.md` Section 4.3 ("Logging") +- `.chief/milestone-2/_contract/base-connector-contract.md` + +## Steps + +1. Create `packages/plugin-env/src/utils.ts` with `maskingValue()` +2. Add `maskValues` field to `EnvConnectorConfig`, default `true` +3. Add a log line in `load()` after storing each value, masked or raw based on `maskValues` +4. Export `maskingValue` from `packages/plugin-env/src/index.ts` if useful to consumers (optional — confirm with reviewer if this should stay internal) +5. Write tests for both `maskValues: true` (default) and `maskValues: false` + +## Acceptance Criteria + +- Default (`maskValues` unset): logged value is masked (e.g. `"8080"` logs as `"8080"` unchanged if ≤4 chars, longer values show first 4 chars + `*`s per `maskingValue()`'s existing behavior) +- `maskValues: false`: logged value is the raw unmasked string +- `pnpm build` exits 0 +- `pnpm check-types` exits 0 +- `pnpm test` exits 0 + +## Verification + +```bash +pnpm build +pnpm check-types +pnpm test --filter @synthing/plugin-env +``` + +## Deliverables + +- `packages/plugin-env/src/utils.ts` (new) +- `packages/plugin-env/src/env-connector.ts` (updated) +- `packages/plugin-env/src/env-connector.test.ts` (updated) +- `packages/plugin-env/src/index.ts` (updated, if exporting `maskingValue`) diff --git a/packages/core/src/connector.ts b/packages/core/src/connector.ts index 05d47b0..ea4e06b 100644 --- a/packages/core/src/connector.ts +++ b/packages/core/src/connector.ts @@ -1,17 +1,35 @@ +import type { Logger } from './context.js'; + /** - * BaseConnector — domain-agnostic abstract connector. + * BaseConnector — domain-agnostic connector contract. * Two-phase interface: load once, get per-key. + * + * Structural (interface, not abstract class) so a connector plugin's + * installed @synthing/core copy doesn't need to be the same class + * instance as the host's — avoids the dual-package hazard. */ -export abstract class BaseConnector { +export interface BaseConnector { + config: Config; + logger?: Logger; + /** * Load / prepare values for the given keys. * Called once with all keys this connector might need (allows batch fetching). */ - abstract load(keys: string[]): Promise; + load(keys: string[]): Promise; /** * Get a single value (sync, called per-key after load). * Returns the raw value or `undefined` when the key is not found. */ - abstract get(key: string): unknown | undefined; + get(key: string): unknown | undefined; + + /** + * Optional — set the working directory for connectors that read local files + * (e.g. EnvConnector's .env lookup). No-op if a connector doesn't need one. + */ + setWorkingDir?(dir: string | undefined): void; + + /** Optional — get the working directory, if this connector supports one. */ + getWorkingDir?(): string | undefined; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1dce99a..1c0e73b 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -4,7 +4,7 @@ * Interfaces and types only. No implementation logic. */ -export { BaseConnector } from './connector.js'; +export type { BaseConnector } from './connector.js'; export { BaseGenerator } from './generator.js'; export type { GeneratorOutput, SerializedOutput } from './generator.js'; export type { GeneratorContext, Logger } from './context.js'; diff --git a/packages/plugin-env/eslint.config.mjs b/packages/plugin-env/eslint.config.mjs new file mode 100644 index 0000000..1f1f207 --- /dev/null +++ b/packages/plugin-env/eslint.config.mjs @@ -0,0 +1,14 @@ +import { config } from "@synthing/config-eslint/base"; + +/** @type {import("eslint").Linter.Config} */ +export default [ + ...config, + { + // Test files freely set arbitrary process.env keys to simulate env vars — + // these aren't real build-time config turbo needs to know about. + files: ["**/*.test.ts"], + rules: { + "turbo/no-undeclared-env-vars": "off", + }, + }, +]; diff --git a/packages/plugin-env/src/env-connector.test.ts b/packages/plugin-env/src/env-connector.test.ts index 01e580a..129d30c 100644 --- a/packages/plugin-env/src/env-connector.test.ts +++ b/packages/plugin-env/src/env-connector.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { EnvConnector } from "./env-connector.js"; describe("EnvConnector", () => { @@ -18,7 +18,7 @@ describe("EnvConnector", () => { }); describe("with prefix", () => { - it("reads env var using prefix + key as-is (no uppercase transform)", async () => { + it("reads env var using prefix + key as-is", async () => { process.env["APP_port"] = "8080"; const connector = new EnvConnector({ prefix: "APP_" }); await connector.load(["port"]); @@ -44,7 +44,7 @@ describe("EnvConnector", () => { }); describe("without prefix", () => { - it("reads env var using key as-is (no uppercase transform)", async () => { + it("reads env var using key as-is", async () => { process.env["port"] = "5000"; const connector = new EnvConnector(); await connector.load(["port"]); @@ -69,29 +69,31 @@ describe("EnvConnector", () => { }); }); - describe("key casing — no uppercase transform", () => { - it("uses key exactly as given (mixed-case)", async () => { - process.env["APP_myKey"] = "value"; - const connector = new EnvConnector({ prefix: "APP_" }); - await connector.load(["myKey"]); - expect(connector.get("myKey")).toBe("value"); - }); - }); - - describe("caseInsensitive option", () => { - it("matches env vars case-insensitively when enabled", async () => { + describe("case-insensitive matching (default)", () => { + it("matches an uppercase env var against a lowercase-declared key by default", async () => { process.env["APP_PORT"] = "9090"; - const connector = new EnvConnector({ prefix: "APP_", caseInsensitive: true }); + const connector = new EnvConnector({ prefix: "APP_" }); await connector.load(["port"]); expect(connector.get("port")).toBe("9090"); }); - it("stores key normalized when caseInsensitive is true", async () => { + it("stores the key normalized by default", async () => { process.env["APP_MYKEY"] = "hello"; - const connector = new EnvConnector({ prefix: "APP_", caseInsensitive: true }); + const connector = new EnvConnector({ prefix: "APP_" }); await connector.load(["myKey"]); expect(connector.get("myKey")).toBe("hello"); }); + + it("can be disabled via caseInsensitive: false, requiring exact case", async () => { + process.env["APP_port"] = "8080"; + const connector = new EnvConnector({ + prefix: "APP_", + caseInsensitive: false, + }); + await expect(connector.load(["PORT"])).rejects.toThrow( + "Missing environment variable: APP_PORT" + ); + }); }); describe("workingDir", () => { @@ -115,28 +117,19 @@ describe("EnvConnector", () => { }); }); - describe("tryParseSecretValue()", () => { - it("returns raw string for plain strings", () => { - const connector = new EnvConnector(); - expect(connector.tryParseSecretValue("hello")).toBe("hello"); - }); - - it("parses flat JSON object", () => { - const connector = new EnvConnector(); - const result = connector.tryParseSecretValue('{"user":"admin","port":5432}'); - expect(result).toEqual({ user: "admin", port: 5432 }); - }); - - it("returns raw string for non-flat JSON (arrays)", () => { - const connector = new EnvConnector(); - const result = connector.tryParseSecretValue("[1,2,3]"); - expect(result).toBe("[1,2,3]"); + describe("raw value passthrough (no coercion)", () => { + it("returns a flat-JSON-looking value as the raw, unparsed string", async () => { + process.env["APP_config"] = '{"user":"admin","port":5432}'; + const connector = new EnvConnector({ prefix: "APP_" }); + await connector.load(["config"]); + expect(connector.get("config")).toBe('{"user":"admin","port":5432}'); }); - it("returns raw string for non-flat JSON (nested objects)", () => { - const connector = new EnvConnector(); - const result = connector.tryParseSecretValue('{"a":{"b":1}}'); - expect(result).toBe('{"a":{"b":1}}'); + it("returns any string untouched, regardless of shape", async () => { + process.env["APP_raw"] = "[1,2,3]"; + const connector = new EnvConnector({ prefix: "APP_" }); + await connector.load(["raw"]); + expect(connector.get("raw")).toBe("[1,2,3]"); }); }); @@ -156,4 +149,33 @@ describe("EnvConnector", () => { expect(connector.get("SKIP_KEY")).toBe("xyz"); }); }); + + describe("maskValues logging", () => { + it("masks the logged value by default", async () => { + process.env["APP_secret"] = "supersecretvalue"; + const infoSpy = vi.fn(); + const connector = new EnvConnector({ prefix: "APP_" }); + connector.logger = { info: infoSpy, warn: vi.fn(), error: vi.fn() }; + await connector.load(["secret"]); + + const loggedValueCall = infoSpy.mock.calls.find((call) => + String(call[0]).startsWith("Value:") + ); + expect(loggedValueCall?.[0]).toBe("Value: supe************"); + expect(loggedValueCall?.[0]).not.toContain("supersecretvalue"); + }); + + it("logs the raw value when maskValues is false", async () => { + process.env["APP_secret"] = "supersecretvalue"; + const infoSpy = vi.fn(); + const connector = new EnvConnector({ prefix: "APP_", maskValues: false }); + connector.logger = { info: infoSpy, warn: vi.fn(), error: vi.fn() }; + await connector.load(["secret"]); + + const loggedValueCall = infoSpy.mock.calls.find((call) => + String(call[0]).startsWith("Value:") + ); + expect(loggedValueCall?.[0]).toBe("Value: supersecretvalue"); + }); + }); }); diff --git a/packages/plugin-env/src/env-connector.ts b/packages/plugin-env/src/env-connector.ts index fcc7736..6539433 100644 --- a/packages/plugin-env/src/env-connector.ts +++ b/packages/plugin-env/src/env-connector.ts @@ -2,15 +2,9 @@ import path from "node:path"; import { config as loadDotenv } from "dotenv"; -import { BaseConnector, type Logger } from "@synthing/core"; +import { type BaseConnector, type Logger } from "@synthing/core"; -/** - * SecretValue — the resolved value of a secret. - * Can be a raw string or a flat JSON object (string/number/boolean/null values). - */ -export type SecretValue = - | string - | Record; +import { maskingValue } from "./utils.js"; export interface EnvConnectorConfig { /** @@ -27,7 +21,7 @@ export interface EnvConnectorConfig { /** * Whether to perform case-insensitive lookups for environment variables. * If true, the connector will match environment variable names in a case-insensitive manner. - * @default false + * @default true */ caseInsensitive?: boolean; @@ -36,38 +30,53 @@ export interface EnvConnectorConfig { * @default process.cwd() */ workingDir?: string; + + /** + * Whether to mask values when logging them. + * The connector has no visibility into which keys are declared secrets + * (that's a VariableManager-level concern), so it masks everything + * indiscriminately by default rather than guessing. + * @default true + */ + maskValues?: boolean; } /** - * EnvConnector — reads secrets/variables from `process.env`, + * EnvConnector — reads variables/secrets from `process.env`, * optionally loading from a .env file and supporting configurable * prefixes and case-insensitive lookups. * - * Key mapping: variable key `"port"` with prefix `"APP_"` reads `process.env.APP_port`. - * (No uppercase transform — the key is used as-is.) + * Key mapping: variable key `"port"` with prefix `"APP_"` looks for + * `process.env.APP_port`, matched case-insensitively by default — so + * `APP_PORT`, `APP_port`, etc. all resolve the same key. + * + * Returns raw string values only. No coercion or parsing happens here — + * that's the engine's job via @synthing/toolkit's `coerceFromString()`, + * which is the only component that knows a key's declared type. * * Implements the two-phase interface: `load()` once, `get()` per-key. */ -export class EnvConnector extends BaseConnector { +export class EnvConnector implements BaseConnector { public config: EnvConnectorConfig; private prefix: string; - private secrets = new Map(); + private values = new Map(); private caseInsensitive: boolean; + private maskValues: boolean; public logger?: Logger; private workingDir?: string; constructor(config: EnvConnectorConfig = {}) { - super(); this.config = config; this.prefix = config.prefix ?? ""; - this.caseInsensitive = config.caseInsensitive ?? false; + this.caseInsensitive = config.caseInsensitive ?? true; + this.maskValues = config.maskValues ?? true; this.workingDir = config.workingDir; } /** * Set the working directory for loading .env files. */ - setWorkingDir(dir: string): void { + setWorkingDir(dir: string | undefined): void { this.workingDir = dir; } @@ -87,8 +96,8 @@ export class EnvConnector extends BaseConnector { } /** - * Load secrets from environment variables. - * @param keys The names of the secrets to load. + * Load values from environment variables. + * @param keys The names of the values to load. * @throws Will throw an error if a required env var is missing. */ async load(keys: string[]): Promise { @@ -98,7 +107,7 @@ export class EnvConnector extends BaseConnector { } for (const key of keys) { - this.logger?.info(`Loading secret: ${key}`); + this.logger?.info(`Loading value: ${key}`); const expectedKey = this.prefix + key; const matchKey = this.caseInsensitive @@ -111,51 +120,26 @@ export class EnvConnector extends BaseConnector { throw new Error(`Missing environment variable: ${expectedKey}`); } + const rawValue = process.env[matchKey]; const storeKey = this.normalizeName(key); - this.secrets.set(storeKey, this.tryParseSecretValue(process.env[matchKey])); - this.logger?.info(`Loaded secret: ${key} -> ${storeKey}`); - } - } - - /** - * Parse a raw env var string into a SecretValue. - * Attempts to parse flat JSON objects; otherwise returns raw string. - */ - tryParseSecretValue(value: string): SecretValue { - try { - const parsed = JSON.parse(value); - - if ( - typeof parsed === "object" && - parsed !== null && - !Array.isArray(parsed) && - Object.values(parsed).every( - (v) => - typeof v === "string" || - typeof v === "number" || - typeof v === "boolean" || - v === null - ) - ) { - return parsed; - } - - return value; - } catch { - return value; + this.values.set(storeKey, rawValue); + this.logger?.info(`Loaded value: ${key} -> ${storeKey}`); + this.logger?.info( + `Value: ${this.maskValues ? maskingValue(rawValue) : rawValue}` + ); } } /** - * Get the value of a loaded secret. - * @param key The name of the secret. - * @throws Will throw if the secret was not loaded (did you call load()?). + * Get the raw string value of a loaded key. + * @param key The name of the value. + * @throws Will throw if the value was not loaded (did you call load()?). */ - override get(key: string): SecretValue { + get(key: string): string { const storeKey = this.normalizeName(key); - if (!this.secrets.has(storeKey)) { + if (!this.values.has(storeKey)) { throw new Error(`Secret '${key}' not loaded. Did you call load()?`); } - return this.secrets.get(storeKey)!; + return this.values.get(storeKey)!; } } diff --git a/packages/plugin-env/src/index.ts b/packages/plugin-env/src/index.ts index 8e5fc1e..0371ee8 100644 --- a/packages/plugin-env/src/index.ts +++ b/packages/plugin-env/src/index.ts @@ -6,4 +6,5 @@ */ export { EnvConnector } from "./env-connector.js"; -export type { EnvConnectorConfig, SecretValue } from "./env-connector.js"; +export type { EnvConnectorConfig } from "./env-connector.js"; +export { maskingValue } from "./utils.js"; diff --git a/packages/plugin-env/src/utils.ts b/packages/plugin-env/src/utils.ts new file mode 100644 index 0000000..cf7cd44 --- /dev/null +++ b/packages/plugin-env/src/utils.ts @@ -0,0 +1,14 @@ +/** + * Mask a string value for safe logging, keeping the first `length` + * characters and replacing the rest with `*`. + */ +export function maskingValue(value: string, length = 4): string { + length = Math.floor(length); + if (length < 0) { + throw new Error('Length must be a non-negative integer'); + } + if (value.length <= length) { + return value + '*'.repeat(length - value.length); + } + return value.slice(0, length) + '*'.repeat(value.length - length); +} diff --git a/packages/synthing/eslint.config.mjs b/packages/synthing/eslint.config.mjs new file mode 100644 index 0000000..98ee5a0 --- /dev/null +++ b/packages/synthing/eslint.config.mjs @@ -0,0 +1,4 @@ +import { config } from "@synthing/config-eslint/base"; + +/** @type {import("eslint").Linter.Config} */ +export default config; diff --git a/packages/toolkit/eslint.config.mjs b/packages/toolkit/eslint.config.mjs new file mode 100644 index 0000000..98ee5a0 --- /dev/null +++ b/packages/toolkit/eslint.config.mjs @@ -0,0 +1,4 @@ +import { config } from "@synthing/config-eslint/base"; + +/** @type {import("eslint").Linter.Config} */ +export default config; diff --git a/packages/toolkit/src/coerce.ts b/packages/toolkit/src/coerce.ts index 3c07901..646b5d8 100644 --- a/packages/toolkit/src/coerce.ts +++ b/packages/toolkit/src/coerce.ts @@ -37,7 +37,7 @@ export function coerceFromString( let parsed: unknown; try { parsed = JSON.parse(value); - } catch (err) { + } catch { throw new TypeError(`Cannot coerce "${value}" to object: invalid JSON`); } if (