fix(config-loader): cacheHitRate no longer exceeds 100%#500
fix(config-loader): cacheHitRate no longer exceeds 100%#500nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the cache hit rate calculation in config-loader.js could exceed 100%. The issue occurred because the denominator used only disk loads (cache misses) instead of total requests.
Changes:
- Updated
getPerformanceMetrics()to calculate cache hit rate ascacheHits / (cacheHits + cacheMisses) × 100 - Added comprehensive unit tests (25 total) for the config-loader module
- Included 2 regression tests specifically validating the fix for issue #499
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/core/config/config-loader.test.js | New test suite with 25 tests covering all config-loader functions and regression tests for bug #499 |
| .aios-core/install-manifest.yaml | Updated manifest with new file hashes, sizes, and generation timestamp |
| .aios-core/core/config/config-loader.js | Fixed cache hit rate formula to use total requests as denominator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR adds comprehensive unit tests for the config-loader module covering all exported functions, configuration validation, and caching behavior. It also fixes a calculation bug in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
The old formula divided cacheHits by loads (disk reads only), which could produce rates above 100% since most requests are cache hits that never touch disk. Use (cacheHits + cacheMisses) as the denominator for a correct percentage. 25 unit tests including 2 regression tests for this fix. Closes SynkraAI#499
020f272 to
60fb685
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/core/config/config-loader.js (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline failure: Manifest needs regeneration.
The CI indicates the manifest is outdated due to a hash mismatch for this file. Run the following to fix:
npm run generate:manifestThen commit the updated manifest file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.aiox-core/core/config/config-loader.js at line 1, The CI failure shows the manifest hash for config-loader.js is out of date; regenerate the package manifest and commit it by running the project script to rebuild manifests (npm run generate:manifest), then add and commit the updated manifest file so the hash for config-loader.js matches the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.aiox-core/core/config/config-loader.js:
- Line 1: The CI failure shows the manifest hash for config-loader.js is out of
date; regenerate the package manifest and commit it by running the project
script to rebuild manifests (npm run generate:manifest), then add and commit the
updated manifest file so the hash for config-loader.js matches the repo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7dc480a-cebf-4784-a871-a79814d61bc2
📒 Files selected for processing (1)
.aiox-core/core/config/config-loader.js
|
Oi @Pedrovaleriolopez @oalanicolas, esse fix é bem direto — corrige o cálculo de hit rate do cache que passava de 100%. Tem teste cobrindo o cenário. Podem dar uma olhada quando tiverem um momento? |
|
@Pedrovaleriolopez @oalanicolas, lembrando que esse fix está aberto desde 24/fev e agora surgiram PRs duplicados (#581, #582) com o mesmo fix. Nosso PR é o original — rebased, testado e pronto pra merge. Podem aprovar? |
Resumo
cacheHitRateemgetPerformanceMetrics()que podia ultrapassar 100%hits / loads— comoloadsconta apenas leituras do disco ehitsconta acessos ao cache, a taxa podia exceder 100%hits / (hits + misses)— denominador correto que representa o total de consultas ao cacheCloses #499
Plano de teste
Summary by CodeRabbit