fix(triage): add llm config to simili.yaml and relax validation#96
fix(triage): add llm config to simili.yaml and relax validation#96
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLLM API key is made optional by removing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Simili Triage ReportNote Quality Score: 8.5/10 (Good) Classification
Quality Improvements
Similar Threads
Generated by Simili Bot |
Root cause: triage was broken in the main repo because .github/simili.yaml had no `llm:` section. Config validation required llm.api_key, so the entire config was discarded and replaced with an empty struct — zeroing out qdrant.collection and causing Qdrant to reject the empty collection name. Two-part fix: - .github/simili.yaml: add explicit `llm:` section reusing GEMINI_API_KEY - config.Validate(): remove llm.api_key from required fields, since the process command already falls back to embedding.api_key when it is unset. Strict validation was silently discarding a valid config instead of gracefully degrading. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
03fe006 to
ddaf496
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/config/config_test.go (1)
181-189:no llm api key is still validcase is currently redundant.
baseConfigalready has an emptyLLM.APIKey, so this case duplicates the existing"all valid"behavior. Consider either removing it or making"all valid"include a non-empty LLM key so this case adds unique coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config/config_test.go` around lines 181 - 189, The test case named "no llm api key is still valid" is redundant because baseConfig already has LLM.APIKey == "", so either remove that test case or make the suite cover the intended difference: update baseConfig to set a non-empty LLM.APIKey and change the "all valid" case to use that non-empty key, leaving the "no llm api key is still valid" case to explicitly set cfg.LLM.APIKey = "" (referencing baseConfig, Config, and LLM.APIKey and the test case names "all valid" and "no llm api key is still valid"); pick one approach and adjust the test cases accordingly to ensure unique coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/config/config_test.go`:
- Around line 181-189: The test case named "no llm api key is still valid" is
redundant because baseConfig already has LLM.APIKey == "", so either remove that
test case or make the suite cover the intended difference: update baseConfig to
set a non-empty LLM.APIKey and change the "all valid" case to use that non-empty
key, leaving the "no llm api key is still valid" case to explicitly set
cfg.LLM.APIKey = "" (referencing baseConfig, Config, and LLM.APIKey and the test
case names "all valid" and "no llm api key is still valid"); pick one approach
and adjust the test cases accordingly to ensure unique coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1593507e-6a89-46f3-81fd-fcb9afbd0da0
📒 Files selected for processing (3)
.github/simili.yamlinternal/core/config/config.gointernal/core/config/config_test.go
There was a problem hiding this comment.
Pull request overview
This PR updates Simili’s configuration validation and repo-default config so missing llm.api_key no longer causes config loading to fail (and downstream Qdrant requests to break due to empty qdrant.collection when callers fall back to an empty config).
Changes:
- Added an
llm:block to.github/simili.yamlto ensure the repo’s own config passes validation. - Relaxed
Config.Validate()by removingllm.api_keyfrom required fields (relying on existing fallback behavior). - Updated config unit tests to reflect
llm.api_keybeing optional and to assert onqdrant.collectionas the required merged field.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/core/config/config.go |
Makes llm.api_key optional during config validation and documents rationale. |
internal/core/config/config_test.go |
Adjusts validation/load tests for optional llm.api_key and merged-config validation expectations. |
.github/simili.yaml |
Adds llm configuration so the repo’s own config validates successfully. |
Comments suppressed due to low confidence (1)
internal/core/config/config_test.go:186
- The new test case "no llm api key is still valid" doesn't currently add coverage because
baseConfigno longer setsLLM.APIKey, so the preceding "all valid" case already exercises the "missing llm.api_key" path. Consider either restoring a non-emptyLLM.APIKeyinbaseConfig(and keeping this test to prove optionality), or removing this case to avoid redundant coverage.
// llm.api_key is optional — process command falls back to embedding.api_key.
name: "no llm api key is still valid",
cfg: func() Config {
cfg := baseConfig
cfg.LLM.APIKey = ""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -219,7 +222,6 @@ func (c *Config) Validate() error { | |||
| {name: "qdrant.api_key", envVar: "QDRANT_API_KEY", value: c.Qdrant.APIKey}, | |||
| {name: "qdrant.collection", envVar: "QDRANT_COLLECTION", value: c.Qdrant.Collection}, | |||
| {name: "embedding.api_key", envVar: "EMBEDDING_API_KEY", value: c.Embedding.APIKey}, | |||
There was a problem hiding this comment.
Validate() currently reports missing embedding.api_key as "check EMBEDDING_API_KEY", but this repo appears to standardize on GEMINI_API_KEY/OPENAI_API_KEY (and .github/simili.yaml uses GEMINI_API_KEY). This makes the validation error message misleading; consider updating the env-var hint (or the validation strategy) to match the actual supported env keys.
| {name: "embedding.api_key", envVar: "EMBEDDING_API_KEY", value: c.Embedding.APIKey}, | |
| {name: "embedding.api_key", envVar: "GEMINI_API_KEY or OPENAI_API_KEY", value: c.Embedding.APIKey}, |
🧪 E2E Test✅ Bot responded: yes | Auto-closer (dry-run) | processed: 0 closed: 0 grace: 0 human: 0 | Test repo → gh-simili-bot/simili-e2e-22701300627 Auto-generated by E2E pipeline |
Summary
llm:block to.github/simili.yamlso the repo's own config passes validation (previously the missingllm.api_keycausedValidate()to discard the entire config, leavingqdrant.collectionempty and breaking Qdrant requests)llm.api_keyfrom the required-fields list inValidate()— theprocesscommand already falls back toembedding.api_keywhenllm.api_keyis unset, so failing hard here was causing more harm than goodllm.api_keyis now a valid config, andTestLoadWithInheritanceValidatesMergedConfignow asserts onqdrant.collection(the actual required field) insteadRoot cause
The E2E tests seed their own config with a full
llm:block, so they never hit this path. The repo's ownsimili.yamlhad nollm:section, causingValidate()to fail → config discarded →qdrant.collection = ""→ Qdrant rejected requests.Test plan
go test ./internal/core/config/...— all 9 tests passsimili.yamlnow contains anllm:block with${GEMINI_API_KEY}Summary by CodeRabbit
New Features
Configuration