fix: address CodeRabbit review findings#24
Merged
giancarloerra merged 2 commits intomainfrom Apr 12, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughA set of small fixes: markdown cleanups, .gitignore update, test count docs updated, env var parsing validation, path-separator normalization for ignore handling, log level normalization, safer embedding error checks, slight Ruby import behavior change, and tightened tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/integration/code-graph.test.ts (1)
113-116: Avoid silent pass by assertinghelpersNodeexists.Good tightening on Line 115, but the
if (helpersNode)guard still lets this test pass when the node is missing.✅ Proposed assertion hardening
- if (helpersNode) { - // If the graph correctly resolves imports, helpers should have dependents - expect(helpersNode.dependents.length).toBeGreaterThan(0); - } + expect(helpersNode).toBeDefined(); + // If the graph correctly resolves imports, helpers should have dependents + expect(helpersNode!.dependents.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/code-graph.test.ts` around lines 113 - 116, The test silently passes when helpersNode is missing because of the conditional guard; replace the if (helpersNode) guard with an explicit assertion that helpersNode exists (e.g., expect(helpersNode).toBeDefined() or .not.toBeNull()) before asserting its dependents, and then proceed to assert helpersNode.dependents.length > 0 so the test fails loudly if the node is absent (update the block referencing helpersNode and its dependents accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DEVELOPER.md`:
- Line 214: The aggregate test count is inconsistent after changing the Unit
count to 608; update the summary totals that still show 634 so they instead
reflect the new table sum of 765 (608 + 137 + 20). Locate the occurrences
mentioning the old aggregate (e.g., the lines that currently state "634") and
replace them with "765" (and ensure any nearby references to per-layer totals
match 608 for unit/, 137 and 20 accordingly); also verify the other instance
referenced (the second occurrence) is updated to the same 765 value so both
aggregates are consistent.
In `@src/services/embedding-config.ts`:
- Around line 132-141: The current parsing of EMBEDDING_DIMENSIONS
(rawDimensions → embeddingDimensions) only checks for NaN and can accept zero or
negative values; update the validation to require a positive integer: after
parsing process.env.EMBEDDING_DIMENSIONS into rawDimensions, verify
Number.isFinite(rawDimensions) && rawDimensions > 0 and throw a clear Error if
not, then assign embeddingDimensions = rawDimensions; apply the identical
stricter positive-int validation to the other analogous parse block (the second
raw.../embedding... parse in this file) so both parsed config values reject zero
or negatives at parse time.
In `@src/services/ignore.ts`:
- Line 130: The shouldIgnore function currently passes relativePath directly
into ig.ignores(), which on Windows can be backslash-delimited and thus miss
rules that expect forward slashes; update shouldIgnore to normalize the incoming
relativePath the same way relDir is normalized (e.g., replace path.sep with "/"
or use relativePath.split(path.sep).join("/")) before calling ig.ignores(), so
ig.ignores(relativePath) receives forward-slash paths; adjust places in
shouldIgnore where ig.ignores is invoked (reference: shouldIgnore, ig.ignores)
and ensure behavior remains consistent with the earlier relDir normalization
used for .gitignore prefixes.
In `@src/services/logger.ts`:
- Around line 28-31: The envLevel lookup uses the `in` operator which checks the
prototype chain; update the validation in the `envLevel` -> `currentLevel` logic
to validate only own properties of `LOG_LEVELS` (e.g., use
`Object.hasOwn(LOG_LEVELS, envLevel)`) and ensure `envLevel` is a string before
casting to `LogLevel`; change the condition in the `currentLevel` assignment
that currently uses `envLevel in LOG_LEVELS` to use the own-property check so
`LOG_LEVELS[currentLevel]` cannot become undefined (refer to `envLevel`,
`LOG_LEVELS`, and `currentLevel`).
---
Nitpick comments:
In `@tests/integration/code-graph.test.ts`:
- Around line 113-116: The test silently passes when helpersNode is missing
because of the conditional guard; replace the if (helpersNode) guard with an
explicit assertion that helpersNode exists (e.g.,
expect(helpersNode).toBeDefined() or .not.toBeNull()) before asserting its
dependents, and then proceed to assert helpersNode.dependents.length > 0 so the
test fails loudly if the node is absent (update the block referencing
helpersNode and its dependents accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1686946-9b7d-4e26-839b-23b69bfe8d85
📒 Files selected for processing (16)
.gitignoreAGENTS.mdCHANGELOG.mdCLAUDE.mdDEVELOPER.mdREADME.mdhooks/hooks.jsonsrc/services/embedding-config.tssrc/services/graph-imports.tssrc/services/ignore.tssrc/services/logger.tssrc/services/provider-google.tssrc/services/provider-openai.tstests/integration/code-graph.test.tstests/unit/query-tools.test.tstests/unit/watcher.test.ts
💤 Files with no reviewable changes (3)
- CLAUDE.md
- AGENTS.md
- CHANGELOG.md
| │ ├── fixtures.ts # Test fixture utilities (temp projects, Docker checks) | ||
| │ └── setup.ts # Integration test infrastructure (Qdrant client, cleanup) | ||
| ├── unit/ # 460 tests — no Docker required | ||
| ├── unit/ # 608 tests — no Docker required |
There was a problem hiding this comment.
Update the aggregate test count to match the new per-layer totals.
After setting Unit to 608, the totals in this file are inconsistent: Line 518 still says 634, but the table now sums to 765 (608 + 137 + 20).
📝 Proposed doc fix
-SocratiCode uses **vitest** as its test framework with **634 tests** across three layers.
+SocratiCode uses **vitest** as its test framework with **765 tests** across three layers.Also applies to: 546-546
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DEVELOPER.md` at line 214, The aggregate test count is inconsistent after
changing the Unit count to 608; update the summary totals that still show 634 so
they instead reflect the new table sum of 765 (608 + 137 + 20). Locate the
occurrences mentioning the old aggregate (e.g., the lines that currently state
"634") and replace them with "765" (and ensure any nearby references to
per-layer totals match 608 for unit/, 137 and 20 accordingly); also verify the
other instance referenced (the second occurrence) is updated to the same 765
value so both aggregates are consistent.
- Use Object.hasOwn() instead of `in` for log level validation - Normalize relativePath in shouldIgnore() for Windows compatibility - Reject zero/negative/decimal values in embedding config (Number() instead of parseInt()) - Update aggregate test count to 765 - Harden code-graph test assertion with explicit toBeDefined()
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
1 participant
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.
Summary
Addresses 16 findings from a full-codebase CodeRabbit scan. Fixes defensive coding gaps in source, strengthens test assertions, and corrects documentation issues. Two findings were rejected as false positives (server.json schema URL and provider-ollama stale config).
Changes
Source fixes:
embedding-config.ts: Add NaN validation forEMBEDDING_DIMENSIONSandEMBEDDING_CONTEXT_LENGTHenv varsgraph-imports.ts: Fix Rubyrequire_relativeincorrectly marked asisDynamic: truelogger.ts: ValidateSOCRATICODE_LOG_LEVELagainst known levels before useprovider-openai.ts: GuardembedSingleagainst empty results; conditionally passdimensionsonly for models that support it (text-embedding-3-*)provider-google.ts: GuardembedSingleagainst empty resultsignore.ts: Normalizepath.relative()separators to forward slashes for cross-platform compatibilityTest fixes:
query-tools.test.ts: Broaden regex to catch all bracketed project tags, not just hyphenated oneswatcher.test.ts: Add missing assertion in "ignores non-indexable files" testcode-graph.test.ts: Replace trivially-truetoBeGreaterThanOrEqual(0)withtoBeGreaterThan(0)Documentation & config fixes:
hooks/hooks.json: Replace em-dash with hyphen in CLI command (copy-paste safe)README.md: Fix stray,:punctuation in headingCLAUDE.md/AGENTS.md: Remove orphan closing code fencesCHANGELOG.md: Remove duplicate# Changelogheader blockDEVELOPER.md: Update unit test counts from 460/477 to actual 608Type of change
Testing
npm run test:unit) — 608/608 passingnpm run test:integration) — if applicablenpx tsc --noEmit)Checklist
Related issues
Findings from CodeRabbit full-codebase review (no linked issue).
Summary by CodeRabbit
Bug Fixes
Documentation
Chores