From f225c7bb59857012a5fac33602ce2ea3848224d3 Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Thu, 14 May 2026 08:25:43 -0400 Subject: [PATCH] fix(config): warn instead of throw on validation failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User report: every coco command crashes with "Invalid project config" when the merged config (defaults + XDG + git + project + env) fails schema validation. The throw originated from `loadProjectJsonConfig` but the offending fields are often inherited from EARLIER layers (most commonly XDG / git config), so the error pinned the blame on the wrong source — and a stale field anywhere in the chain takes down the whole tool. ## What was broken \`\`\` Error: Invalid project config cause: data/service must NOT have additional properties, data/service/requestOptions/timeout must be number, data/service/requestOptions/maxRetries must be number, data/service must have required property 'endpoint', data/service must match a schema in anyOf \`\`\` Reproducible against the user's own XDG config (the service shape has drifted from the strict schema — extra fields + wrong number types). Adding ANY local \`.coco.config.json\` triggered re-validation of the merged result, which inherited the invalid XDG service. Every \`coco ui\`, \`coco log\`, \`coco commit\` ... crashed during boot. ## What this PR changes \`loadProjectJsonConfig\` no longer throws on validation failure. Instead: 1. Logs a clear warning to stderr with: - The path of the local file that triggered the validation - The specific schema errors - A note that the validation runs against the MERGED config — the issue may be in any of: defaults, XDG, git, project, env (not necessarily the local file) 2. Applies the merge anyway so the user's intent flows through 3. Lets downstream components guard locally against shapes they can't handle (clearer error than "tool failed to boot") ## Why this is the right answer - Throwing turns "you have a stale field somewhere in your config" into "the whole tool fails to boot." That's a brutal regression cost for a recoverable situation. - Coco has sane defaults at every layer. A drifted service shape can fall back to default service behavior and the rest of the tool keeps working — commits, changelogs, PR flows, etc. - If a future consumer GENUINELY can't operate without a valid service shape, it can throw a localized error at the moment of use with concrete next steps, rather than burying the user in an opaque "Invalid project config" at boot. ## Verified - Local repro of the user's exact error: \`coco log --repo /tmp/conv-test\` (with their config that triggered the bug) now warns + outputs the log instead of crashing. - New regression test: \`warns instead of throwing when the merged config fails validation\`. ## Out of scope The drifted schema itself — the user's working config genuinely has fields the schema rejects. Could be the schema is too strict (recent service-config additions not yet reflected) OR genuine drift in the user's file. Worth a follow-up audit of the service schema vs what real configs look like in the wild. This PR just stops the bleed. ## Tests - 1666 tests pass (one flaky parallel-execution test unrelated) - New regression test covers the warn-not-throw path --- src/lib/config/services/project.test.ts | 39 ++++++++++++++++++++++++ src/lib/config/services/project.ts | 40 +++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/lib/config/services/project.test.ts b/src/lib/config/services/project.test.ts index 5618d899..cef14531 100644 --- a/src/lib/config/services/project.test.ts +++ b/src/lib/config/services/project.test.ts @@ -56,4 +56,43 @@ describe('loadProjectConfig', () => { expect(config.service.endpoint).toBe('http://localhost:11434') } }) + + it('warns instead of throwing when the merged config fails validation', () => { + // Reproduces the user-reported crash: the merged config has fields + // the schema rejects (often inherited from a stale XDG / git + // config layer), and the previous behavior threw, taking down + // every coco command. The fix surfaces a warning so the user + // can fix the offending field but keeps the merge so the rest + // of the tool stays usable. + mockFs.existsSync.mockReturnValue(true) + // Project file has a valid local override (conventionalCommits) + // but the merged result will inherit an invalid service shape + // from the incoming `config` argument. + mockFs.readFileSync.mockReturnValue( + JSON.stringify({ conventionalCommits: true }) + ) + + const invalidIncoming = { + service: { + // Missing required fields + extra unknown field — guaranteed + // to fail any anyOf branch in the service schema. + provider: 'openai', + model: 'gpt-4o', + unknownDriftField: 'something', + }, + } as unknown as Config + + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) + + expect(() => loadProjectJsonConfig(invalidIncoming)).not.toThrow() + + const result = loadProjectJsonConfig(invalidIncoming) + // Local override still merged in despite validation failure — + // the tool keeps working with the user's intent applied. + expect((result as unknown as { conventionalCommits?: boolean }).conventionalCommits).toBe(true) + expect(warnSpy).toHaveBeenCalled() + expect(warnSpy.mock.calls[0][0]).toContain('config validation issues detected') + + warnSpy.mockRestore() + }) }) diff --git a/src/lib/config/services/project.ts b/src/lib/config/services/project.ts index 6377a30c..0566a7c2 100644 --- a/src/lib/config/services/project.ts +++ b/src/lib/config/services/project.ts @@ -45,12 +45,46 @@ export function loadProjectJsonConfig( fs.readFileSync(resolvedPath, 'utf-8') ) as Partial & { $schema: string } - config = { ...config, ...projectConfig } as Config + const merged = { ...config, ...projectConfig } as Config - const isProjectConfigValid = validate(config) + // Validate the merged result, but DON'T throw on failure. + // Reasons: + // + // 1. Throwing turns "you have a stale field somewhere in your + // config" into "the whole tool fails to boot" — every coco + // command that loads config crashes. That's a brutal + // regression cost for what's almost always a recoverable + // situation. + // + // 2. The validation runs on the MERGED config, so the actual + // offending file might be the XDG / git / env layer, not + // the local project file we're nominally loading. Pinning + // the error to "project config" was actively misleading. + // + // 3. We have sane defaults. A drifted service shape can fall + // back to the default service and the rest of the tool + // keeps working — the user still gets their commits, their + // changelogs, their PR flows, etc. + // + // Instead: warn loudly to stderr (with the file path AND the + // specific schema errors) so the user knows exactly what to fix, + // and apply the merge so the user's intent still flows through. + // If a downstream component genuinely can't operate without a + // valid service shape, it can guard against that locally with a + // clear error rather than blowing up at config-load time. + const isProjectConfigValid = validate(merged) if (!isProjectConfigValid) { - throw new Error('Invalid project config', { cause: ajv.errorsText(validate.errors) }) + const errors = ajv.errorsText(validate.errors) + console.warn( + `[coco] Warning: config validation issues detected (continuing with merged config).\n` + + ` Local file: ${resolvedPath}\n` + + ` Schema issues: ${errors}\n` + + ` Fix the offending fields to silence this warning. The validation runs ` + + `against the merged result of every config source (defaults, XDG, git, project, env) ` + + `— the issue may be in any of those, not necessarily this file.` + ) } + config = merged } if (opts?.returnSource) {