chore: full codebase review trigger - #21
Closed
ZeR020 wants to merge 1 commit into
Closed
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
@coderabbitai review |
Owner
Author
|
@coderabbitai summary |
Owner
Author
|
@coderabbitai full review src/ |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (95)
📝 WalkthroughWalkthroughThis PR adds audit marker comments ( ChangesAudit Instrumentation and Type Declarations
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
✅ Actions performedSummary regeneration triggered. |
✅ Actions performedReview triggered.
|
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/logger.ts`:
- Line 117: Remove the non-functional audit comment line "// audit:
src/services/logger.ts" from the logger module; this is a dead comment and
should be deleted so the file (and any exported logger functions/objects in
src/services/logger.ts) has no unnecessary LOC added by the audit marker.
In `@tests/project-scope.test.ts`:
- Line 78: Remove the non-functional "// audit:" comment markers (e.g., "//
audit: tests/project-scope.test.ts") from this file and across the codebase; if
audit metadata is required, migrate it out of source files into CI/GitHub
Actions or a review-tracking system and, where necessary, replace any marker
that was documenting intent with a meaningful in-source comment describing the
rationale (refer to occurrences of the "// audit:" marker to locate all
instances such as in project-scope.test.ts).
---
Outside diff comments:
In `@src/services/ai/opencode-provider.ts`:
- Around line 218-244: The streaming pull handler duplicates the same regex
replacement in two places (when flushing remaining buffer and when emitting
lines) which wastes CPU; extract the replacement into a small helper like
stripMcpPrefix(text) and call that helper wherever the regex is applied
(references: the ReadableStream pull method, the local variables buffer and
textToEmit, and the regex /"name"\s*:\s*"mcp_([^"]+)"/g) so each text segment is
transformed once before enqueueing or before final flush.
In `@src/services/ai/providers/google-gemini.ts`:
- Around line 88-96: The loop that builds parts from msg.toolCalls (the for
(const tc of msg.toolCalls) block) and the separate tool-call validation block
both call JSON.parse(tc.function.arguments) without protection; wrap each
JSON.parse in a try/catch, and on parse failure set a safe fallback (e.g., null
or the raw string) and record or surface a structured error so the provider
continues iteration instead of throwing; update the handling in the parts
construction and the tool-call validation (the places that reference
tc.function.arguments and the validation logic around line ~232) to use the
try/catch-wrapped parse and propagate a clear validation error object/message
for malformed JSON.
- Around line 98-111: The JSON.parse call for tool responses
(JSON.parse(msg.content) inside the msg.role === "tool" branch) can throw on
corrupted content; wrap that parse in a try/catch, log the parse error, and push
a safe fallback into contents instead of letting the provider crash: attempt to
parse msg.content, and on success keep the current structure
(functionResponse.response = parsed), but on failure set
functionResponse.response to the raw msg.content (or an object like {
parseError: true, raw: msg.content }) and include a logged error referencing the
tool name ((msg.toolCallId || "").split(":")[0]) so the code in google-gemini.ts
continues rather than throwing.
In `@src/services/client.ts`:
- Around line 107-110: The log call in src/services/client.ts currently includes
the full user query (variable query) which may contain sensitive data; update
the log invocation in the embedding fallback path (the log(...) call that takes
{ query, error: String(error) }) to omit the raw query and instead log safe
metadata (e.g., query length, a truncated/masked version, or a hash) along with
the error; ensure you modify the same log(...) invocation so that only
non-sensitive fields (like { queryLength: query?.length, queryHash:
safeHash(query), error: String(error) } or similar) are emitted.
In `@src/services/jsonc.ts`:
- Around line 6-85: The stripJsoncComments function currently builds the output
via repeated string concatenation (result += char) which is O(n²); change to an
array-based accumulator (e.g., let out: string[] = []) and push characters/lines
instead of using result += in the bodies that reference result (the occurrences
inside the quote-handling block, general character append, inside
inSingleLineComment/newline handling, and inMultiLineComment newline handling).
After the loop, join the array (out.join('')) and run the trailing-comma regex
on that joined string before returning; keep all existing branching/state logic
(inString, inSingleLineComment, inMultiLineComment) and only replace the string
mutations with array pushes and a final join.
In `@src/services/logger.ts`:
- Around line 29-40: The rotateLog function currently swallows all errors in its
catch block; update the catch to capture the thrown error and write a
descriptive error message to stderr so filesystem issues are visible in
production. Specifically, in rotateLog (which calls getLogFilePath, uses
MAX_LOG_SIZE, and manipulates oldLog), replace the empty catch with code that
accepts the error (e.g., catch (err)) and logs a clear message including the
logFile path and the error object to console.error (stderr) so permission/disk
errors during existsSync/statSync/renameSync/unlinkSync are surfaced.
- Around line 56-67: Move the inline regex in safeStringify into a precompiled
module-level constant (e.g., SENSITIVE_KEY_REGEX =
/token|secret|password|api[-_]?key|authorization/i) and use that constant inside
safeStringify instead of rebuilding the RegExp on each call; also change the
empty catch to capture the thrown error (e.g., catch (err)) and return a
descriptive string containing the error message/stack (for example
'"[Unserializable data: <error.message>]"') so serialization failures aren’t
silently swallowed.
- Around line 19-23: Replace the manual slash parsing in getLogDirPath() with
Node's path.dirname: call path.dirname(getLogFilePath()) to compute the
directory reliably (and remove the lastIndexOf logic); also ensure the module
has the Node path import (e.g., import path from "path" or const path =
require("path")) so getLogDirPath and getLogFilePath use the built-in path
utilities.
- Around line 92-100: Replace the blocking appendFileSync call in logWithLevel
with an async, ordered write queue using fs/promises.appendFile: keep shouldLog,
ensureLoggerInitialized, getLogFilePath, safeStringify and the existing log line
construction, then enqueue writes (e.g., a module-level Promise chain or a small
buffer/flush mechanism) to call appendFile(logFile, line) to preserve ordering
and handle write errors (log or stderr) without throwing; ensure the queue
handles backpressure and exposes a graceful flush on shutdown if needed so logs
are persisted before exit.
In `@src/services/migration-service.ts`:
- Around line 307-334: The returned deletedShards value currently uses
mismatch.shardMismatches.length which overstates deletions when shards are kept
due to re-embedding failures; update the logic to track actual deletions by
incrementing a counter (e.g., deletedShardsCount) whenever
shardManager.deleteShard(shardInfo.shardId) is successfully called (inside the
if (!shardHadFailures) block) and return that counter instead of
mismatch.shardMismatches.length; ensure the counter is initialized before the
shard loop and used in the final return alongside existing reEmbeddedCount and
duration.
- Around line 66-76: The current migration check only compares storedDimensions
to CONFIG.embeddingDimensions and misses cases where the embedding model changed
but dimensions stayed the same; update the conditional in the block that builds
mismatches (the if using storedDimensions and CONFIG.embeddingDimensions) to
also check storedModel against the expected model (e.g., CONFIG.embeddingModel)
so a mismatch is detected when either dimensions or model differ, keep
vectorSearch.countAllVectors(db) and the mismatches push (including storedModel
and vectorCount) intact so the mismatch entry reflects both differences.
In `@src/services/privacy.ts`:
- Around line 5-8: isFullyPrivate currently returns false when
stripPrivateContent produces multiple adjacent "[REDACTED]" tokens; update
isFullyPrivate to treat any string composed solely of one or more "[REDACTED]"
tokens (with optional whitespace between them) as fully private by normalizing
or testing for that pattern. Specifically, in isFullyPrivate (referencing
stripPrivateContent), after getting stripped, either collapse consecutive
"[REDACTED]" tokens into a single marker or check with a regex like
/^(\[REDACTED\]\s*)+$/ to return true, and still return true for an empty
string.
In `@src/services/sqlite/sqlite-bootstrap.ts`:
- Around line 119-125: The empty catch on the bun:sqlite probe silently swallows
errors; update the try/catch around getRequire()("bun:sqlite") so the catch
captures the error (e.g., catch (err)) and only silently fall back to
BetterSqlite3Database when the error indicates the module is missing (module not
found), otherwise log the full error (using the project logger or console)
before falling back; reference the existing symbols DatabaseImpl,
BunSqliteDatabase, BetterSqlite3Database and getRequire() to locate the probe
and add error-type checking and a logged message in the catch.
- Around line 38-50: The wrapper methods run, get, and all currently use
redundant ternaries checking params.length (e.g., run: (...params: unknown[]) =>
{ const result = params.length > 0 ? stmt.run(...params) : stmt.run(); }), so
simplify each to call the underlying stmt directly with the spread args (e.g.,
stmt.run(...params)), removing the params.length conditional; apply this same
change to the corresponding methods in both database wrapper classes where
run/get/all are defined so the wrappers delegate directly to
stmt.run(...params), stmt.get(...params), and stmt.all(...params).
- Around line 27-115: Both BunSqliteDatabase and BetterSqlite3Database duplicate
the same wrapper logic (prepare/run/get/all/exec/close); refactor by extracting
a single generic class (e.g., SqliteDatabase implements Database) that accepts
an underlying DB instance in its constructor and implements prepare, run, exec,
and close once, then replace BunSqliteDatabase and BetterSqlite3Database
constructors with simple factories that load the module via getRequire() (the
existing getRequire("bun:sqlite") and getRequire("better-sqlite3") calls) and
pass the created DB instance into the generic SqliteDatabase; keep existing
method names (prepare, run, exec, close) and Statement shape so callers remain
unchanged.
In `@src/services/sqlite/vector-search.ts`:
- Around line 72-80: Extract the duplicated 28-field INSERT SQL into a single
private constant (e.g., MEMORIES_INSERT_SQL) at the class level and replace the
inline SQL strings in insertVector, batchInsertVectors, and replaceVector with
references to that constant; ensure the constant contains the exact parameter
placeholders and fields currently used so behavior is unchanged and update any
tests or callers if they referenced the previous inline strings.
- Around line 246-249: The instance-level constants MIN_OVER_FETCH,
MAX_OVER_FETCH, TARGET_FILL_RATIO, and BASE_MULTIPLIER in class VectorSearch
should be converted to static readonly properties to avoid per-instance
duplication; change their declarations to static readonly and update all
internal references to use the class-qualified form (e.g.,
VectorSearch.MIN_OVER_FETCH) instead of this.MIN_OVER_FETCH so callers and
methods reference the shared constants.
- Around line 208-219: The insertPromises array is being given Promises wrapped
in Promise.resolve unnecessarily; remove the redundant Promise.resolve calls and
push the Promises returned directly from backend.insert(...) for both the
content insert and the optional tags insert (check the insertPromises pushes
that call backend.insert with { id: record.id, vector: record.vector, shard,
kind: "content" } and the conditional that uses record.tagsVector with kind:
"tags" and push those backend.insert(...) results directly).
- Around line 640-651: Hoist the wordSetCache and getWordSet out of
searchAcrossShards to module-level or the containing class so the cache is
reused across calls: move the const wordSetCache = new Map<string,
Set<string>>() and the function getWordSet(text: string) to the class/module
scope (e.g., as private getWordSet(...) and a private wordSetCache property),
remove the nested definition inside searchAcrossShards, and update calls inside
searchAcrossShards to use this.getWordSet(...) (or getWordSet(...) if
module-level); also consider adding a cache size limit or TTL to wordSetCache to
avoid unbounded growth.
- Around line 266-534: searchWithMultiplier is too large and mixes concerns;
split out FTS5 lookup, hydration/scoring, and access-count updates into focused
helpers: implement searchFTS5(db, queryText, limit) to encapsulate the FTS5 +
LIKE fallback logic currently inside searchWithMultiplier, implement
hydrateAndScoreResults(rows, scoreMap, queryText, context, ftsResults) to
perform tag parsing, exactMatchBoost, ftsBoost, multi-factor scoring,
contextBoost and return hydrated SearchResult objects, and implement
updateAccessCounts(db, results) to run the UPDATE loop; then replace the
corresponding blocks in searchWithMultiplier to call these helpers (keep
scoreMap and ids logic in-place) so functionality is identical but the main
function is shorter and more testable.
- Around line 883-887: The rows mapping that splits tags and parses metadata is
duplicated in the json_extract and LIKE fallback branches; extract that logic
into a local helper (e.g., mapRowToResult or transformRow) declared before the
try-catch in src/services/sqlite/vector-search.ts and replace both occurrences
(the rows.map(...) blocks) with a call to rows.map(mapRowToResult), ensuring the
helper uses row.tags ? row.tags.split(",") : [] and
safeParseMetadata(row.metadata) || {} to produce the same shape.
- Around line 126-128: The empty catch blocks around db.run("ROLLBACK") silently
swallow errors, reducing observability. Update these catch blocks in the
transaction rollback sections in the vector-search service (including lines
126-128, 743-745, and 819-821) to log rollback failures using the existing
logger or a similar mechanism to capture error details. This will improve error
visibility and aid debugging.
In `@src/services/tags.ts`:
- Around line 21-108: All git helper functions (getGitEmail, getGitName,
getGitRepoUrl, getGitCommonDir, getGitTopLevel) swallow exceptions and return
null, making failures hard to debug; refactor by extracting a shared
execGitCommand helper that runs execSync with the same options, catches errors,
and logs the caught error via the project's logger (or a debug flag) before
returning null, then replace each function to call execGitCommand (passing cwd
where used) to preserve the null contract while improving observability.
- Around line 144-189: Introduce memoization for getUserTagInfo and
getProjectTagInfo to avoid repeated git shell calls: add a module-level
cachedUserTag: TagInfo | undefined and cachedProjectTags: Map<string, TagInfo>,
have getUserTagInfo return cachedUserTag if present (compute and set it
otherwise), have getProjectTagInfo check cachedProjectTags.get(directory) and
set it after computing, and add a clearTagCache (or clearTagsCache) function to
invalidate both cachedUserTag and cachedProjectTags for tests or when config
changes; reference functions getUserTagInfo, getProjectTagInfo and export
clearTagCache so callers/tests can refresh the cache.
- Around line 21-108: The five functions getGitEmail, getGitName, getGitRepoUrl,
getGitCommonDir and getGitTopLevel duplicate execSync setup and try/catch logic;
create a single helper (e.g., runGitCommand or execGit) that accepts the git
args and an optional cwd and returns the trimmed string or null on error, move
the shared options (encoding, stdio, timeout) into that helper, then refactor
each function to call this helper; preserve the special post-processing in
getGitCommonDir (checking isAbsolute, normalize/resolve, existsSync and
realpathSync) after using the helper to obtain the raw commonDir.
- Around line 139-142: The getProjectName function uses parts[parts.length - 1]
to pick the last path segment; replace that legacy index access with the ES2022
Array.prototype.at method to simplify and clarify intent: split the directory
into parts as currently done, then return parts.at(-1) ?? directory (or fallback
to directory if result is falsy). Update references in getProjectName and keep
the same split/filter logic.
- Around line 21-108: The current functions getGitEmail, getGitName,
getGitRepoUrl, getGitCommonDir, and getGitTopLevel use execSync and block the
event loop; refactor them to async equivalents (e.g., getGitEmailAsync,
getGitNameAsync, getGitRepoUrlAsync, getGitCommonDirAsync, getGitTopLevelAsync)
by using promisified child_process.exec (or util.promisify(exec)) and await the
command, preserve existing options (cwd, timeout, encoding) and the same
null-on-error semantics, and ensure path handling in getGitCommonDirAsync still
resolves/isAbsolute/realpathSync logic but using async fs variants where
appropriate to avoid blocking.
In `@src/services/user-profile/profile-context.ts`:
- Around line 19-43: The code assumes profileData.preferences,
profileData.patterns, and profileData.workflows are arrays and calls
.length/.sort/.slice/.forEach on them; if parsed JSON is malformed or legacy
these can be undefined and crash. Fix by normalizing or guarding these fields
before use: ensure profileData.preferences, profileData.patterns, and
profileData.workflows are arrays (e.g., replace non-arrays with [] or use
Array.isArray checks / optional chaining) prior to the blocks that push entries
(the blocks referencing profileData.preferences, profileData.patterns, and
profileData.workflows), so sort/slice/forEach only run on actual arrays.
In `@src/services/utils/safe-transforms.ts`:
- Around line 6-29: The safeToISOString function currently creates multiple Date
instances in the string parsing and fallback paths; refactor safeToISOString to
create at most one Date per call by: 1) early-return for null/undefined as now,
2) convert timestamp to a single numeric candidate and if that is not finite and
typeof timestamp === "string" try constructing one Date from the string once, 3)
otherwise construct one Date from the numeric candidate, and 4) validate that
single Date with isNaN(date.getTime()) and return its toISOString() or the
fallback new Date().toISOString(); update the logic inside safeToISOString to
reuse that single Date variable instead of creating parsedDate and another Date
in fallbacks.
In `@src/web/app.js`:
- Around line 513-535: The current bulk-delete always shows success and clears
UI even if fetchAPI calls for "/api/prompts/bulk-delete" or
"/api/memories/bulk-delete" fail; update the logic around promptIds/memoryIds
handling to aggregate results and only show the success toast, clear
state.selectedMemories, and call loadMemories/loadStats/updateBulkActions when
both API calls succeeded (or at least no errors occurred), otherwise show an
error toast (with error details) and do NOT clear selections; use the existing
symbols promptIds, memoryIds, fetchAPI, result.success, deletedCount, showToast,
state.selectedMemories.clear, loadMemories, loadStats, and updateBulkActions to
implement this conditional flow and surface failures to the user.
- Around line 752-758: In showTagMigrationModal, the overlay element
("tag-migration-overlay") is read but never made visible; update the function to
remove the hidden state (e.g., remove the "hidden" attribute and/or remove a
"hidden" CSS class) from the overlay and, if present, ensure any aria-hidden or
display styles are updated so the modal becomes visible; keep the existing
setting of status.textContent and the start button hookup
(start-tag-migration-btn → runTagMigration).
- Around line 814-830: The migration-section element is located but never made
visible in showMigrationWarning, so after populating migration-message the UI
stays hidden; update showMigrationWarning to unhide the section (e.g., remove
any hidden CSS class and/or set section.hidden = false or clear
section.style.display) after setting message.textContent, make sure to target
the same "migration-section" element and optionally guard that
"migration-message" exists before updating.
In `@tests/privacy.test.ts`:
- Around line 48-73: Add a test for isFullyPrivate that covers multiple adjacent
private tags: call isFullyPrivate with a string like
"<private>one</private><private>two</private>" (and a variant with surrounding
whitespace) and assert it returns true; place the new it block alongside the
existing isFullyPrivate tests and mirror the style of the other cases so it
verifies the function correctly handles adjacent private tags without touching
other tests or global state.
In `@tests/profile-tool-runtime.test.ts`:
- Around line 45-52: The createPlugin helper mutates shared module-level state
(currentTags.user) and reuses a shared tmpDir, which prevents safe parallel
tests; refactor by making each test create its own temp directory (use
mkdtempSync in a beforeEach or createPlugin param) and avoid mutating
currentTags globally—pass tag values into createPlugin as parameters and/or
replace the global mock with per-test mocks using vi.doMock inside
beforeEach/afterEach so createPlugin, WARMUP_KEY and tmpDir are test-local and
safe for parallel execution.
In `@tests/profile-write.test.ts`:
- Around line 12-24: The tests mutate module-level tmpDir and CONFIG.storagePath
which causes races; change makeManager (and remove the module-level tmpDir) so
it creates a unique temp directory per call (e.g., mkdtemp or include a
random/test-id suffix) and sets CONFIG.storagePath to that unique path before
dynamically importing UserProfileManager, or refactor UserProfileManager
construction to accept an injected storagePath so tests no longer mutate CONFIG;
update references to tmpDir, makeManager, CONFIG.storagePath and
UserProfileManager accordingly so each test gets an isolated DB path and tests
can run in parallel (alternatively mark the suite describe.sequential() if you
intentionally want to disable parallelization).
In `@tests/project-scope.test.ts`:
- Around line 14-32: The helper createRepoWithWorktree runs many real git execs
and slows tests — either mock git/fs for unit tests or optimize the helper for
integration tests: for unit tests, replace createRepoWithWorktree use in
getProjectTagInfo tests with a mocked filesystem or stub the run/execSync calls
to return expected git metadata (simulate .git/HEAD, refs and .git/worktrees) so
you test logic without calling git; for integration tests, modify
createRepoWithWorktree to reuse a cached initialized repo across tests or reduce
commands (e.g., initialize once, reuse repoDir and only add lightweight
worktrees) and add an optional parameter like reuseRepo=true to avoid repeating
git init/config/commit on every call. Ensure references to
createRepoWithWorktree and getProjectTagInfo are updated accordingly.
In `@tests/schema-version.test.ts`:
- Around line 24-108: Replace on-disk DB files with in-memory SQLite databases
to speed tests: update tests that create dbPath and new DbClass(...) /
connectionManager.getConnection(...) (references:
connectionManager.getConnection, new DbClass(...), runMigrations,
getCurrentVersion) to use ':memory:' or 'file::memory:?cache=shared' where
persistent filesystem behavior isn't required; for tests that need multiple
connections to the same in-memory DB use the shared URI, and ensure any
setup/teardown still calls close() so runMigrations and getCurrentVersion
operate against the in-memory connection.
In `@tests/semantic-dedup-ingest.test.ts`:
- Around line 98-120: The tests currently inspect internals via getDbForShard,
getMemoryById, and countMemories (using shardManager, connectionManager and
direct SQL) which couples them to storage details; replace those helpers with
assertions against the public LocalMemoryClient API (use methods like
list/search/getById or other retrieval/search endpoints) to verify memory
presence, counts and contents, and remove any direct SQL/shardManager usage so
tests validate behavior rather than implementation.
- Around line 213-214: Remove the flaky setTimeout-based synchronization; delete
the await new Promise((r) => setTimeout(r, 50)) and either (A) mock the
timestamp source so you can deterministically advance time around the operation
that sets created_at/updated_at, or (B) change the assertion that uses
toBeGreaterThan on updated_at/created_at to a non-strict comparison (e.g., use
toBeGreaterThanOrEqual / toBeGreaterThanOrEqual assertion) so the test does not
rely on wall-clock sleeps; update the test around the updated_at/created_at
checks accordingly.
- Around line 55-96: Remove the duplicated test lifecycle hooks and keep a
single canonical set: collapse the two identical beforeEach blocks that set
(CONFIG as any).deduplicationIngestEnabled, (CONFIG as
any).deduplicationSimilarityThreshold and initialize client = new
LocalMemoryClient() into one beforeEach; collapse the two identical afterEach
comments about not calling connectionManager.closeAll() into one afterEach; and
merge the two afterAll blocks into a single afterAll that restores (CONFIG as
any).storagePath = originalStoragePath and drains tempDirs with rmSync, ensuring
originalStoragePath is restored exactly once; update or keep comments about
shardManager.metadataDb and unique containerTag hashes as needed.
- Around line 63-68: The tests currently skip connectionManager.closeAll() in
the afterEach to keep shardManager.metadataDb open, which risks connection leaks
and prevents parallelization; modify the lifecycle so each test cleans up
connections but still leaves metadataDb available: implement either a
reference-counted/open-on-demand pattern in connectionManager (increment on
shardManager.openMetadataDb() and decrement/close only when refcount hits zero)
or explicitly call connectionManager.closeAll() in afterEach and reopen
shardManager.metadataDb in beforeEach (e.g., via shardManager.openMetadataDb()
or a new shardManager.initMetadataDbFor(containerTag)); update the afterEach and
any shared setup code to use the new open/close calls so tests remain isolated
and can run in parallel while preserving metadataDb accessibility.
In `@tests/tags.test.ts`:
- Around line 1-32: Current tests only cover getProjectName; add unit tests for
the high-level identity/tag functions to exercise behavior and edge cases.
Create tests for getUserTagInfo, getProjectTagInfo, and getTags in
tests/tags.test.ts (or a new file) that mock the underlying git command calls
used by getGitEmail, getGitName, getGitRepoUrl, getGitCommonDir, getGitTopLevel,
getProjectRoot and getProjectIdentity (use Vitest's mocking/stubbing for the
module or the child_process/exec wrapper the code uses) to return expected
values and to simulate failures (missing git, permission error, unusual repo
structure). Assert the produced tag objects/strings and error-handling behavior
(e.g., fallbacks, partial tags) rather than implementation details so
getUserTagInfo/getProjectTagInfo/getTags produce correct outputs across success
and error scenarios.
In `@tests/vector-backends/migration-fallback.test.ts`:
- Around line 14-19: The afterEach cleanup currently deletes entries from
tempDirs with rmSync but doesn’t close the test database connection created
earlier; update the test to track the DB client (the variable that receives the
DB/connection instance created in the test) and close it before directory
removal: add a reference (or push the instance into a connections array) and in
afterEach call the connection’s proper shutdown method (e.g.,
close()/disconnect()/dispose()) prior to running the existing while loop that
uses tempDirs and rmSync so files aren’t removed while the DB is still open.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3840458-36d5-4470-a79d-68ff1528870d
📒 Files selected for processing (95)
src/config.tssrc/index.tssrc/plugin.tssrc/services/ai/ai-provider-factory.tssrc/services/ai/opencode-provider.tssrc/services/ai/provider-config.tssrc/services/ai/providers/anthropic-messages.tssrc/services/ai/providers/base-provider.tssrc/services/ai/providers/google-gemini.tssrc/services/ai/providers/openai-chat-completion.tssrc/services/ai/providers/openai-responses.tssrc/services/ai/session/ai-session-manager.tssrc/services/ai/session/session-types.tssrc/services/ai/tools/tool-schema.tssrc/services/ai/validators/user-profile-validator.tssrc/services/api-handlers.tssrc/services/auto-capture.tssrc/services/cleanup-service.tssrc/services/client.tssrc/services/context.tssrc/services/deduplication-service.tssrc/services/embedding.tssrc/services/jsonc.tssrc/services/language-detector.tssrc/services/logger.tssrc/services/memory-conflicts.tssrc/services/memory-lifecycle.tssrc/services/memory-scoring-service.tssrc/services/memory-scoring.tssrc/services/migration-service.tssrc/services/platform-server.tssrc/services/privacy.tssrc/services/retrieval-context.tssrc/services/secret-resolver.tssrc/services/sqlite/connection-manager.tssrc/services/sqlite/schema.tssrc/services/sqlite/shard-manager.tssrc/services/sqlite/sqlite-bootstrap.tssrc/services/sqlite/transcript-manager.tssrc/services/sqlite/types.tssrc/services/sqlite/vector-search.tssrc/services/tags.tssrc/services/transcript-capture.tssrc/services/user-memory-learning.tssrc/services/user-profile/profile-context.tssrc/services/user-profile/profile-utils.tssrc/services/user-profile/types.tssrc/services/user-profile/user-profile-manager.tssrc/services/user-prompt/user-prompt-manager.tssrc/services/utils/safe-transforms.tssrc/services/vector-backends/backend-factory.tssrc/services/vector-backends/exact-scan-backend.tssrc/services/vector-backends/nsw-backend.tssrc/services/vector-backends/types.tssrc/services/vector-backends/usearch-backend.tssrc/services/web-server.tssrc/types/index.tssrc/types/usearch.d.tssrc/web/app.jssrc/web/i18n.jstests/adaptive-overfetch.test.tstests/ai-provider-config.test.tstests/anthropic-provider.test.tstests/config-resolution.test.tstests/config.test.tstests/contextual-decay.test.tstests/embedding-cache-api.test.tstests/embedding-cache.test.tstests/embedding-degradation.test.tstests/language-detector.test.tstests/memory-engine.test.tstests/memory-scope.test.tstests/memory-scoring.test.tstests/nsw-backend.test.tstests/openai-chat-completion-provider.test.tstests/opencode-provider.test.tstests/plugin-loader-contract.test.tstests/privacy.test.tstests/profile-tool-runtime.test.tstests/profile-write.test.tstests/project-scope.test.tstests/query-aware-injection.test.tstests/schema-version.test.tstests/semantic-dedup-ingest.test.tstests/structured-format.test.tstests/tags.test.tstests/token-budget.test.tstests/tool-scope.test.tstests/vector-backends/backend-factory.test.tstests/vector-backends/exact-scan-backend.test.tstests/vector-backends/migration-fallback.test.tstests/vector-backends/usearch-backend.test.tstests/vector-search-backend-integration.test.tstests/wal-batch.test.tstests/windows-path.test.ts
| export function error(message: string, data?: unknown) { | ||
| logWithLevel("error", message, data); | ||
| } | ||
| // audit: src/services/logger.ts |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove non-functional audit comment.
The audit marker adds no runtime or documentation value and increases LOC unnecessarily. Version control already tracks file history and changes.
♻️ Remove the audit comment
-// audit: src/services/logger.ts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // audit: src/services/logger.ts |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/logger.ts` at line 117, Remove the non-functional audit comment
line "// audit: src/services/logger.ts" from the logger module; this is a dead
comment and should be deleted so the file (and any exported logger
functions/objects in src/services/logger.ts) has no unnecessary LOC added by the
audit marker.
| expect(rootTag.projectPath).toBe(nestedTag.projectPath); | ||
| }); | ||
| }); | ||
| // audit: tests/project-scope.test.ts |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Reconsider the audit comment markers across the codebase.
The // audit: comment provides no functional or documentary value. If this is intended to track code review coverage, consider using external tooling (CI checks, GitHub Actions, or review management tools) rather than adding 92 non-functional comments across the codebase. Source comments should convey intent, explain non-obvious logic, or document decisions—not serve as metadata markers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/project-scope.test.ts` at line 78, Remove the non-functional "//
audit:" comment markers (e.g., "// audit: tests/project-scope.test.ts") from
this file and across the codebase; if audit metadata is required, migrate it out
of source files into CI/GitHub Actions or a review-tracking system and, where
necessary, replace any marker that was documenting intent with a meaningful
in-source comment describing the rationale (refer to occurrences of the "//
audit:" marker to locate all instances such as in project-scope.test.ts).
Owner
Author
|
Full codebase review complete. All findings captured. Closing trigger PR without merge. |
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.
Full codebase review request. Please review all files in src/
Summary by CodeRabbit