From 4dd72b164ca96cc5a6abc4d86a8e7e97136580a3 Mon Sep 17 00:00:00 2001 From: Jason Mulligan Date: Mon, 22 Jun 2026 14:19:09 -0400 Subject: [PATCH] =?UTF-8?q?Revert=20"feat:=20config=20audit=20fixes=20?= =?UTF-8?q?=E2=80=94=20error=20handling,=20state=20divergence,=20dead=20co?= =?UTF-8?q?de,=20input=20validation=20(#409)"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 0a07d24aed9c894aeb31293dc53e9d6fecc7abe3. --- .../changes/config-audit-fixes/.openspec.yaml | 2 - openspec/changes/config-audit-fixes/design.md | 84 ------------------- .../changes/config-audit-fixes/proposal.md | 27 ------ .../specs/config-module-fixes/spec.md | 42 ---------- openspec/changes/config-audit-fixes/tasks.md | 44 ---------- 5 files changed, 199 deletions(-) delete mode 100644 openspec/changes/config-audit-fixes/.openspec.yaml delete mode 100644 openspec/changes/config-audit-fixes/design.md delete mode 100644 openspec/changes/config-audit-fixes/proposal.md delete mode 100644 openspec/changes/config-audit-fixes/specs/config-module-fixes/spec.md delete mode 100644 openspec/changes/config-audit-fixes/tasks.md diff --git a/openspec/changes/config-audit-fixes/.openspec.yaml b/openspec/changes/config-audit-fixes/.openspec.yaml deleted file mode 100644 index 38f76288..00000000 --- a/openspec/changes/config-audit-fixes/.openspec.yaml +++ /dev/null @@ -1,2 +0,0 @@ -schema: spec-driven -created: 2026-06-22 diff --git a/openspec/changes/config-audit-fixes/design.md b/openspec/changes/config-audit-fixes/design.md deleted file mode 100644 index d8ee2191..00000000 --- a/openspec/changes/config-audit-fixes/design.md +++ /dev/null @@ -1,84 +0,0 @@ -## Context - -The `./src/config` module handles configuration loading, validation, mutation, and persistence. It consists of three files: -- `loader.js` — YAML parsing, env var resolution, deep merge, config load/save/mutate -- `mutate.js` — Dot-path assignment and validation for config mutations -- `schemas.js` — Zod schemas for config validation and DEFAULT_CONFIG - -The audit-code scan identified five issues: missing error handling in YAML parsing, in-memory/disk state divergence on write failure, duplicate code, dead schemas, and missing input validation. - -## Goals / Non-Goals - -**Goals:** -- Eliminate crash risk from malformed config.yaml -- Eliminate in-memory/disk state divergence in setConfigValue() -- Remove duplicate _parseValue() implementation -- Remove three dead provider schemas -- Add input validation guard to assignPath() -- Add test coverage for all new behavior - -**Non-Goals:** -- No changes to ConfigSchema structure or DEFAULT_CONFIG values -- No new configuration options or capabilities -- No changes to env var resolution logic (only the parseValue source changes) -- No changes to ProvidersSchema.passthrough() behavior - -## Decisions - -### 1. Malformed YAML → fallback to DEFAULT_CONFIG -**Decision:** Treat malformed config.yaml the same as missing config.yaml — fall back to DEFAULT_CONFIG. -**Rationale:** A malformed file is as bad as no file. The operator needs the app to start, not crash. Logging the error gives visibility without blocking startup. -**Alternatives considered:** -- Throw and crash: worse UX, breaks startup -- Merge what we can: complex, error-prone, YAML parser doesn't support partial parse - -### 2. Persist-first mutation order in setConfigValue() -**Decision:** Write to disk first, then mutate in-memory. On write failure, throw without mutating. -**Rationale:** The current order (mutate then persist) creates a window where memory and disk disagree. Reversing the order eliminates this window entirely. The trade-off is that a successful mutation now requires a successful disk write first, which is slightly slower but correct. -**Alternatives considered:** -- Keep current order and add rollback: complex, error-prone, what if rollback also fails? -- Use a transaction log: overkill for a single config file -- Write to temp file then rename: atomic on POSIX, but adds complexity for marginal gain - -### 3. Import parseValue from mutate.js -**Decision:** Remove _parseValue() from loader.js, import parseValue from mutate.js. -**Rationale:** Byte-identical functions are a maintenance burden. mutate.js already exports parseValue, so the import is trivial. -**Alternatives considered:** -- Keep both: violates DRY, future divergences likely -- Move parseValue to a shared utils module: over-engineering for one function - -### 4. Remove dead provider schemas -**Decision:** Remove _OpenaiProviderConfigSchema, _OpenrouterProviderConfigSchema, _FalProviderConfigSchema. -**Rationale:** They are prefixed with `_` (private), never referenced in ConfigSchema or DEFAULT_CONFIG, and ProvidersSchema.passthrough() accepts any provider config anyway. They serve no purpose. -**Alternatives considered:** -- Keep them commented out: dead code is still dead code -- Reference them in ProvidersSchema: unnecessary complexity, passthrough already handles it - -### 5. assignPath() object guard -**Decision:** Throw descriptive Error if obj is null, undefined, or not an object. Allow arrays (they are objects in JS). -**Rationale:** The function assumes obj is a valid target for dot-path assignment. Without validation, callers get confusing errors deep in the loop. A guard at the top gives clear feedback. -**Alternatives considered:** -- Coerce to object: silent coercion hides bugs -- Return false on invalid input: changes the function's contract (it throws on other errors) - -## Risks / Trade-offs - -| Risk | Mitigation | -|------|-----------| -| Persist-first order means write failures block mutations | This is the point — we want mutations to fail if they can't be persisted | -| Importing parseValue from mutate.js creates a circular dependency risk | mutate.js imports from schemas.js, loader.js imports from both — no cycle | -| Removing dead schemas might surprise future developers who expect them | The `_` prefix and lack of references make it clear they were never used | -| assignPath() guard might break unexpected callers | Low risk — assignPath is internal, called only from applyDotPathMutation which always passes a valid object | - -## Migration Plan - -No migration needed. All changes are internal implementation fixes: -1. Deploy to non-production environment first -2. Verify config.yaml loads correctly (both present and absent cases) -3. Verify setConfigValue() persists and mutates correctly -4. Verify existing tests pass -5. Rollback: revert branch — no data migration or schema changes to undo - -## Open Questions - -None. All five findings are well-scoped with clear fixes. \ No newline at end of file diff --git a/openspec/changes/config-audit-fixes/proposal.md b/openspec/changes/config-audit-fixes/proposal.md deleted file mode 100644 index eeb42089..00000000 --- a/openspec/changes/config-audit-fixes/proposal.md +++ /dev/null @@ -1,27 +0,0 @@ -## Why - -The audit-code scan identified five precision issues in `./src/config` that introduce crash risks, state divergence, dead code, and missing input validation. These are not new features — they are fixes to eliminate specific failure modes that can corrupt state or crash the application on startup. - -## What Changes - -- **loader.js:** Wrap `yaml.load()` in try/catch; fall back to `DEFAULT_CONFIG` on parse failure -- **loader.js:** Reverse mutation/persist order in `setConfigValue()` — persist first, mutate second; throw on write failure without mutating memory -- **loader.js:** Remove duplicate `_parseValue()`, import `parseValue` from `mutate.js` instead -- **schemas.js:** Remove three dead provider schemas (`_OpenaiProviderConfigSchema`, `_OpenrouterProviderConfigSchema`, `_FalProviderConfigSchema`) -- **mutate.js:** Add object guard to `assignPath()` — throw descriptive Error for null/undefined/non-object inputs -- **Tests:** Add tests for malformed YAML fallback, assignPath guard, and setConfigValue memory preservation - -## Capabilities - -### New Capabilities - - -### Modified Capabilities - - -## Impact - -- **Files modified:** `src/config/loader.js`, `src/config/mutate.js`, `src/config/schemas.js` -- **Files added:** Test cases for new behavior -- **Breaking changes:** None. All fixes preserve existing behavior for valid inputs. -- **Risk:** Low. Each fix is isolated to a single function. No API changes. \ No newline at end of file diff --git a/openspec/changes/config-audit-fixes/specs/config-module-fixes/spec.md b/openspec/changes/config-audit-fixes/specs/config-module-fixes/spec.md deleted file mode 100644 index 96daca30..00000000 --- a/openspec/changes/config-audit-fixes/specs/config-module-fixes/spec.md +++ /dev/null @@ -1,42 +0,0 @@ -## ADDED Requirements - -### Requirement: Config loader handles malformed YAML gracefully -The system SHALL catch YAML parse errors during config loading and fall back to DEFAULT_CONFIG. - -#### Scenario: Malformed YAML file exists -- **WHEN** config.yaml exists but contains invalid YAML syntax -- **THEN** loadConfig() logs the error and returns DEFAULT_CONFIG without crashing - -#### Scenario: Valid YAML file exists -- **WHEN** config.yaml exists and contains valid YAML -- **THEN** loadConfig() parses, merges with DEFAULT_CONFIG, resolves env vars, and validates as before - -### Requirement: Config mutation persists before mutating memory -The system SHALL write config to disk before mutating the in-memory config object. - -#### Scenario: Write succeeds -- **WHEN** setConfigValue() is called with valid input and disk write succeeds -- **THEN** in-memory config is mutated and returns true - -#### Scenario: Write fails -- **WHEN** setConfigValue() is called and writeFileSync throws -- **THEN** in-memory config is unchanged and an Error is thrown - -### Requirement: assignPath validates object input -The system SHALL validate that assignPath() receives a non-null object argument. - -#### Scenario: null input -- **WHEN** assignPath(null, "a.b", 1) is called -- **THEN** a descriptive Error is thrown - -#### Scenario: undefined input -- **WHEN** assignPath(undefined, "a.b", 1) is called -- **THEN** a descriptive Error is thrown - -#### Scenario: non-object input -- **WHEN** assignPath("string", "a.b", 1) is called -- **THEN** a descriptive Error is thrown - -#### Scenario: valid object input -- **WHEN** assignPath({}, "a.b", 1) is called -- **THEN** the value is assigned correctly \ No newline at end of file diff --git a/openspec/changes/config-audit-fixes/tasks.md b/openspec/changes/config-audit-fixes/tasks.md deleted file mode 100644 index 51fb8be3..00000000 --- a/openspec/changes/config-audit-fixes/tasks.md +++ /dev/null @@ -1,44 +0,0 @@ -## 1. Fix YAML parse error handling in loadConfig() - -- [ ] 1.1 Wrap yaml.load(fileContent) in try/catch in loader.js loadConfig() -- [ ] 1.2 On parse error: log error via console.error, fall back to DEFAULT_CONFIG -- [ ] 1.3 On success: proceed with deep merge and env resolution as before - -## 2. Fix in-memory/disk divergence in setConfigValue() - -- [ ] 2.1 Refactor setConfigValue() to persist to disk first via saveConfig() -- [ ] 2.2 On write success: mutate in-memory config via applyDotPathMutation() -- [ ] 2.3 On write failure: throw descriptive Error without mutating in-memory config - -## 3. Remove duplicate _parseValue() from loader.js - -- [ ] 3.1 Import parseValue from ./mutate.js in loader.js -- [ ] 3.2 Replace _parseValue() call in _resolveEnvRecursively() with parseValue() -- [ ] 3.3 Remove _parseValue() function definition from loader.js - -## 4. Remove dead provider schemas from schemas.js - -- [ ] 4.1 Remove _OpenaiProviderConfigSchema (lines 67-76) -- [ ] 4.2 Remove _OpenrouterProviderConfigSchema (lines 78-81) -- [ ] 4.3 Remove _FalProviderConfigSchema (lines 83-86) -- [ ] 4.4 Verify no references remain in ConfigSchema or DEFAULT_CONFIG - -## 5. Add object guard to assignPath() in mutate.js - -- [ ] 5.1 Add guard at top of assignPath() that checks obj is non-null object -- [ ] 5.2 Handle typeof null === "object" quirk explicitly -- [ ] 5.3 Throw descriptive Error for null, undefined, string, number inputs -- [ ] 5.4 Allow arrays (they are valid objects in JS) - -## 6. Add tests for new behavior - -- [ ] 6.1 Test: loadConfig() with malformed YAML falls back to DEFAULT_CONFIG and logs error -- [ ] 6.2 Test: assignPath() throws descriptive Error for null, undefined, string, number -- [ ] 6.3 Test: setConfigValue() does not mutate in-memory config when writeFileSync fails -- [ ] 6.4 Test: setConfigValue() works normally when write succeeds (regression) - -## 7. Verify all tests pass - -- [ ] 7.1 Run npm run test and verify all tests pass -- [ ] 7.2 Run npm run lint and verify no lint errors -- [ ] 7.3 Run npm run coverage and verify coverage is maintained \ No newline at end of file