fix(config/git): merge gitconfig overrides instead of replacing the service object#951
Merged
Conversation
…ervice object
Root-cause for the schema-validation warnings users (including the reporter) hit on every coco invocation when they have a [coco] section in ~/.gitconfig. The bug was in loadGitConfig — three related shape problems on the rebuilt service object:
## What was broken
1. **Service REPLACED, not merged.** When ~/.gitconfig had a [coco] section, the loader built a fresh service object from gitconfig keys and assigned it wholesale — wiping default fields like \`tokenLimit\`, \`temperature\`, \`maxConcurrent\`, \`minTokensForSummary\`, \`maxFileTokens\` whenever the gitconfig didn't spell them out. A minimal gitconfig (\`serviceProvider\` + \`serviceModel\` + \`serviceApiKey\`) would end up with a hollow service shape.
2. **requestOptions: { timeout: null, maxRetries: null }.** The loader did \`Number(gitConfigParsed.coco?.serviceRequestOptionsTimeout)\` unconditionally. When that key wasn't set, \`Number(undefined) === NaN\`, which JSON-serializes to \`null\`. The schema requires \`timeout: number\` — null fails.
3. **endpoint + baseURL always attached.** \`endpoint\` is for ollama, \`baseURL\` is for openai. The loader set both fields regardless of provider. Each provider variant in the schema has \`additionalProperties: false\`, so the irrelevant one was rejected on every anyOf branch.
Combined effect: every coco command running against a repo with this gitconfig logged \`[coco] Warning: config validation issues detected\` and (before #949) actually crashed.
## What this PR changes
\`loadGitConfig\` builds an overrides object containing ONLY the keys the user actually set in gitconfig, then merges those on top of the existing service:
\`\`\`ts
const overrides: Record<string, unknown> = {}
if (coco.serviceProvider) overrides.provider = coco.serviceProvider
if (coco.serviceModel) overrides.model = coco.serviceModel
const tokenLimit = numberOrUndefined(coco.serviceTokenLimit)
if (tokenLimit !== undefined) overrides.tokenLimit = tokenLimit
// ... same pattern for every numeric field
service = { ...(service || {}), ...overrides } as LLMService
\`\`\`
Provider-specific keys (baseURL for openai, endpoint for ollama) only attach when the effective provider matches:
\`\`\`ts
const effectiveProvider = (coco.serviceProvider || service?.provider) as ...
if (effectiveProvider === 'openai' && coco.serviceBaseURL) overrides.baseURL = coco.serviceBaseURL
if (effectiveProvider === 'ollama' && coco.serviceEndpoint) overrides.endpoint = coco.serviceEndpoint
\`\`\`
\`requestOptions\` only attaches when at least one sub-field has a finite number value. A \`numberOrUndefined\` helper centralizes the "convert iff valid number, else undefined" logic so the same trap doesn't reappear on a new field.
## Tests
+4 regression tests covering each bug:
- preserves default service fields when gitconfig only sets a few keys
- does not attach requestOptions when neither sub-field is set
- attaches only the requestOptions sub-fields that gitconfig actually sets
- does not attach provider-irrelevant fields (endpoint on openai)
1677 tests pass (no flaky failure this run), lint + tsc clean.
## Verified end-to-end
Reproduced the original failure locally — a ~/.gitconfig with only serviceProvider / serviceModel / serviceApiKey + an empty \`{ "conventionalCommits": true }\` local config. Before this PR: validation warning + hollowed-out service. After this PR: validation passes, service has every default field intact, gitconfig's apiKey override applied correctly.
## Follow-up not in this PR
#949 made schema-validation failures non-fatal (warn + continue). That defense-in-depth still applies — this PR removes the warning's root cause, but if a future config drift surfaces a new schema mismatch, the user still sees a clear warning instead of a crash. Both layers are useful.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Root cause of the schema-validation warnings users see on every coco invocation when they have a [coco] section in ~/.gitconfig. The bug had been in `loadGitConfig` for a while.
Three latent bugs in one block
Service REPLACED, not merged. When ~/.gitconfig had a `[coco]` section, the loader built a fresh service object and assigned it wholesale — wiping default fields like `tokenLimit`, `temperature`, `maxConcurrent`, etc. whenever gitconfig didn't spell them out. A minimal gitconfig left a hollow service shape.
`requestOptions: { timeout: null, maxRetries: null }`. The loader called `Number(gitConfigParsed.coco?.serviceRequestOptionsTimeout)` unconditionally. With the key unset, `Number(undefined) === NaN`, which JSON-serializes to `null`. Schema requires `timeout: number` — null fails.
`endpoint` + `baseURL` always attached. `endpoint` is for ollama, `baseURL` is for openai. Loader set both regardless of provider. Each variant has `additionalProperties: false`, so the irrelevant one breaks the anyOf match.
Combined effect: every coco command logged `[coco] Warning: config validation issues detected` and (before #949) crashed outright.
The fix
Build a sparse overrides object containing ONLY keys the user actually set in gitconfig, then merge on top of the existing service:
```ts
const overrides: Record<string, unknown> = {}
if (coco.serviceProvider) overrides.provider = coco.serviceProvider
if (coco.serviceModel) overrides.model = coco.serviceModel
const tokenLimit = numberOrUndefined(coco.serviceTokenLimit)
if (tokenLimit !== undefined) overrides.tokenLimit = tokenLimit
// ... same pattern for every numeric field
// Provider-specific keys only when the chosen provider matches:
if (effectiveProvider === 'openai' && coco.serviceBaseURL) overrides.baseURL = coco.serviceBaseURL
if (effectiveProvider === 'ollama' && coco.serviceEndpoint) overrides.endpoint = coco.serviceEndpoint
// requestOptions only when at least one sub-field has a finite number:
if (Object.keys(requestOptionsOverrides).length > 0) overrides.requestOptions = requestOptionsOverrides
service = { ...(service || {}), ...overrides } as LLMService
```
A `numberOrUndefined` helper centralizes the "convert iff valid number, else undefined" logic so the trap doesn't reappear on a future field.
Verified end-to-end
Reproduced the user's failure locally — a ~/.gitconfig with only `serviceProvider` / `serviceModel` / `serviceApiKey` plus a minimal `{ "conventionalCommits": true }` repo config.
Before this PR:
```
[coco] Warning: config validation issues detected (continuing with merged config).
Schema issues: data/service must NOT have additional properties,
data/service/requestOptions/timeout must be number, ...
```
After this PR: validation passes silently, service has every default field intact, gitconfig's apiKey override applied correctly.
Tests
+4 regression tests:
1677 tests pass, lint + tsc clean.
Follow-up not in this PR
#949 made schema-validation failures non-fatal (warn + continue). That defense-in-depth still applies — this PR removes the dominant root cause, but if a future config drift surfaces a new schema mismatch, the user still sees a clear warning instead of a crash. Both layers are useful.