fix: harden memory services and web UI#9
Merged
Merged
Conversation
…t markers - api-handlers.ts: Replace O(N²) tag-based shard resolution in global search with direct unique-shard iteration (user + project shards, deduped by dbPath) - web-server-worker.ts: Add API key auth for non-localhost access, PII redaction on all JSON responses (stripPrivateContent + email regex + sensitive key redaction) - Clean up 83 AUDIT_MARKER/AUDIT_TRIGGER comments left from CodeRabbit review - Typecheck passes, no test regressions (101 pass / 46 fail / 7 errors baseline)
…st errors
- XSS: escapeHtml on profile.displayName, profile.message, memory.memoryType (2x)
in web/app.js to prevent script injection from user-controlled data
- Error handling: replace 6 silent .catch(() => {}) swallowers in index.ts with
.catch((err) => log(...)) so TUI toast failures are visible in logs
Owner
Author
|
@coderabbitai Please perform a comprehensive full-repo audit focusing on security, type safety, error handling, resource leaks, logic bugs, cross-platform issues, and test coverage. Review all files thoroughly — this branch contains fixes from multiple prior audit rounds and we need a final verification pass. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves many non-functional AUDIT_MARKER artifacts and stray tokens; adds API-key auth and PII redaction to web-server-worker, tightens search/pagination, exposes two singletons, adds tags to generateSummary, improves toast error logging and HTML escaping, and updates tests. ChangesMass cleanup (markers & stray tokens)
Web-server Worker — Privacy, Auth, Redaction
Search & Pagination (API handlers + web-server)
Singleton exports & data-shape addition
Observability & UI escaping
Tests — cleanup and small edits
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker as WebServerWorker
participant Server as HTTPServer
participant Privacy as PrivacyUtil
Client->>Worker: HTTP request (/api/...), include Host header and x-opencode-mem-key?
Worker->>Worker: isLocalhost(host) check
alt apiKey configured and request not from localhost
Worker->>Worker: validate x-opencode-mem-key
alt mismatch
Worker-->>Client: 401 Unauthorized (redacted jsonResponse)
else match
Worker->>Server: forward request
Server-->>Worker: response data
Worker->>Privacy: redactPII(response data)
Privacy-->>Worker: redacted data
Worker-->>Client: JSON response (redacted)
end
else auth not required
Worker->>Server: forward request
Server-->>Worker: response data
Worker->>Privacy: redactPII(response data)
Privacy-->>Worker: redacted data
Worker-->>Client: JSON response (redacted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Tip For best results, initiate chat on the files or code changes.
Kicking off the full audit now — results will be posted as inline review comments. [full_review] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b053f635
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+512
to
+515
| const userShards = shardManager.getAllShards("user", ""); | ||
| const projectShards = shardManager.getAllShards("project", ""); | ||
| const uniqueTags = new Set<string>(); | ||
| for (const shard of projectShards) { | ||
| const db = connectionManager.getConnection(shard.dbPath); | ||
| const tags = vectorSearch.getDistinctTags(db); | ||
| for (const t of tags) { | ||
| if (t.container_tag) uniqueTags.add(t.container_tag); | ||
| } | ||
| } | ||
| for (const containerTag of uniqueTags) { | ||
| const { scope, hash } = extractScopeFromTag(containerTag); | ||
| const shards = shardManager.getAllShards(scope, hash); | ||
| for (const shard of shards) { | ||
| try { | ||
| const results = await vectorSearch.searchInShard( | ||
| shard, | ||
| queryVector, | ||
| containerTag, | ||
| pageSize | ||
| ); | ||
| memoryResults.push(...results); | ||
| } catch (error) { | ||
| log("Shard search error", { shardId: shard.id, error: String(error) }); | ||
| } | ||
| const searchedPaths = new Set<string>(); | ||
| for (const shard of [...userShards, ...projectShards]) { |
There was a problem hiding this comment.
When the web UI searches with no selected tag, it calls /api/search without tag from the project memory view, and handleListMemories/LocalMemoryClient both treat the untagged/all-projects case as project-only. Adding userShards here means any migrated mem_user_* memories are now searched with an empty containerTag and returned in that project UI, exposing personal/user-scope memories and making them selectable for project memory actions. Please keep this branch to project shards unless the API/UI explicitly requests a user scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR primarily removes // AUDIT_MARKER placeholders across the repo and includes a small set of security/perf hardening changes (notably in the web UI rendering, shard search logic, and the web-server worker).
Changes:
- Remove
// AUDIT_MARKERlines from many source, test, and script files. - Web UI: escape additional user-controlled fields to reduce XSS risk (
memoryType, profile messages/display names). - Backend: optimize unscoped search to avoid N×M shard/tag scans; add API-key auth + response redaction in the web-server worker; log previously-swallowed toast errors.
Reviewed changes
Copilot reviewed 81 out of 82 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/windows-path.test.ts | Remove prior audit marker. |
| tests/vector-search-backend-integration.test.ts | Remove prior audit marker. |
| tests/vector-backends/usearch-backend.test.ts | Remove prior audit marker. |
| tests/vector-backends/migration-fallback.test.ts | Remove prior audit marker. |
| tests/vector-backends/exact-scan-backend.test.ts | Remove prior audit marker. |
| tests/vector-backends/backend-factory.test.ts | Remove prior audit marker. |
| tests/tool-scope.test.ts | Remove prior audit marker. |
| tests/tags.test.ts | Remove prior audit marker. |
| tests/project-scope.test.ts | Remove prior audit marker. |
| tests/profile-write.test.ts | Remove prior audit marker. |
| tests/profile-tool-runtime.test.ts | Remove prior audit marker. |
| tests/privacy.test.ts | Remove prior audit marker. |
| tests/plugin-loader-contract.test.ts | Remove prior audit marker. |
| tests/opencode-provider.test.ts | Remove prior audit marker. |
| tests/openai-chat-completion-provider.test.ts | Remove prior audit marker. |
| tests/memory-scope.test.ts | Remove prior audit marker. |
| tests/memory-engine.test.ts | Remove prior audit marker. |
| tests/language-detector.test.ts | Remove prior audit marker. |
| tests/config.test.ts | Remove prior audit marker. |
| tests/config-resolution.test.ts | Remove prior audit marker. |
| tests/anthropic-provider.test.ts | Remove prior audit marker. |
| tests/ai-provider-config.test.ts | Remove prior audit marker. |
| src/web/i18n.js | Remove prior audit marker. |
| src/web/app.js | Escape additional rendered fields to mitigate XSS in the web UI. |
| src/types/usearch.d.ts | Remove prior audit marker. |
| src/types/index.ts | Remove prior audit marker. |
| src/services/web-server.ts | Remove prior audit marker. |
| src/services/web-server-worker.ts | Add API key auth + PII redaction for JSON responses in worker server. |
| src/services/vector-backends/usearch-backend.ts | Remove prior audit marker. |
| src/services/vector-backends/types.ts | Remove prior audit marker. |
| src/services/vector-backends/exact-scan-backend.ts | Remove prior audit marker. |
| src/services/vector-backends/backend-factory.ts | Remove prior audit marker. |
| src/services/user-prompt/user-prompt-manager.ts | Remove prior audit marker. |
| src/services/user-profile/user-profile-manager.ts | Remove prior audit marker. |
| src/services/user-profile/types.ts | Remove prior audit marker. |
| src/services/user-profile/profile-utils.ts | Remove prior audit marker. |
| src/services/user-profile/profile-context.ts | Remove prior audit marker. |
| src/services/user-memory-learning.ts | Remove prior audit marker. |
| src/services/transcript-capture.ts | Remove prior audit marker. |
| src/services/tags.ts | Remove prior audit marker. |
| src/services/sqlite/vector-search.ts | Remove prior audit marker. |
| src/services/sqlite/types.ts | Remove prior audit marker. |
| src/services/sqlite/transcript-manager.ts | Remove prior audit marker. |
| src/services/sqlite/sqlite-bootstrap.ts | Remove prior audit marker. |
| src/services/sqlite/shard-manager.ts | Remove prior audit marker. |
| src/services/sqlite/connection-manager.ts | Remove prior audit marker. |
| src/services/secret-resolver.ts | Remove prior audit marker. |
| src/services/retrieval-context.ts | Remove prior audit marker. |
| src/services/privacy.ts | Remove prior audit marker. |
| src/services/platform-server.ts | Remove prior audit marker. |
| src/services/migration-service.ts | Remove prior audit marker. |
| src/services/memory-scoring.ts | Remove prior audit marker. |
| src/services/memory-scoring-service.ts | Remove prior audit marker. |
| src/services/memory-lifecycle.ts | Remove prior audit marker. |
| src/services/memory-conflicts.ts | Remove prior audit marker. |
| src/services/language-detector.ts | Remove prior audit marker. |
| src/services/jsonc.ts | Remove prior audit marker. |
| src/services/embedding.ts | Remove prior audit marker. |
| src/services/deduplication-service.ts | Remove prior audit marker. |
| src/services/context.ts | Remove prior audit marker. |
| src/services/client.ts | Remove prior audit marker. |
| src/services/cleanup-service.ts | Remove prior audit marker. |
| src/services/auto-capture.ts | Remove prior audit marker. |
| src/services/api-handlers.ts | Replace N×M shard/tag scan with single-pass search over shards. |
| src/services/ai/validators/user-profile-validator.ts | Remove prior audit marker. |
| src/services/ai/tools/tool-schema.ts | Remove prior audit marker. |
| src/services/ai/session/session-types.ts | Remove prior audit marker. |
| src/services/ai/session/ai-session-manager.ts | Remove prior audit marker. |
| src/services/ai/providers/openai-responses.ts | Remove prior audit marker. |
| src/services/ai/providers/openai-chat-completion.ts | Remove prior audit marker. |
| src/services/ai/providers/google-gemini.ts | Remove prior audit marker. |
| src/services/ai/providers/base-provider.ts | Remove prior audit marker. |
| src/services/ai/providers/anthropic-messages.ts | Remove prior audit marker. |
| src/services/ai/provider-config.ts | Remove prior audit marker. |
| src/services/ai/opencode-provider.ts | Remove prior audit marker. |
| src/services/ai/ai-provider-factory.ts | Remove prior audit marker. |
| src/plugin.ts | Remove prior audit marker. |
| src/index.ts | Replace swallowed toast errors with logging. |
| src/config.ts | Remove prior audit marker. |
| scripts/migrate-v1-to-v2.ts | Remove prior audit marker. |
| scripts/migrate-tests.mjs | Remove prior audit marker. |
| scripts/build.mjs | Remove prior audit marker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+94
to
+105
| // Auth: enforce API key for non-localhost access | ||
| const requiresAuth = | ||
| workerConfig.apiKey && | ||
| workerConfig.enabled && | ||
| workerConfig.host && | ||
| !isLocalhost(workerConfig.host); | ||
| if (requiresAuth) { | ||
| const apiKey = req.headers.get("x-opencode-mem-key"); | ||
| if (apiKey !== workerConfig.apiKey) { | ||
| return jsonResponse({ success: false, error: "Unauthorized" }, 401); | ||
| } | ||
| } |
Comment on lines
+73
to
+81
| if (value && typeof value === "object") { | ||
| const result: Record<string, unknown> = {}; | ||
| for (const [k, v] of Object.entries(value)) { | ||
| // Redact known sensitive keys entirely | ||
| if (/token|secret|password|api[-_]?key|authorization|refresh|access/i.test(k)) { | ||
| result[k] = "[REDACTED]"; | ||
| } else { | ||
| result[k] = redactPII(v); | ||
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/web-server-worker.ts (1)
370-380:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRefuse non-localhost startup when no API key is configured.
Right now
serve()still starts on0.0.0.0/::withmessage.apiKeyunset, which makesrequiresAuthfalse and exposes the full API anonymously. Please reject the start request before binding the socket.🔐 Suggested guard
case "start": { if (server) { self.postMessage({ type: "error", error: "Server already running", } as WorkerResponse); return; } + + if (message.host && !isLocalhost(message.host) && !message.apiKey) { + self.postMessage({ + type: "error", + error: "An API key is required when binding the web server to a non-localhost host", + } as WorkerResponse); + return; + } workerConfig = { host: message.host, apiKey: message.apiKey, enabled: message.enabled,🤖 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/web-server-worker.ts` around lines 370 - 380, Before calling serve() and binding the socket, validate the requested host/port and API key: if message.apiKey is missing/empty and the requested hostname (message.host) is not a localhost address (e.g., not "localhost", "127.0.0.1", "::1"), reject the start request and do not assign workerConfig or call serve(); surface a clear error (throw or return) so server is never created. Update the logic around workerConfig, message.apiKey, and the call to serve() (and any code that sets server) to perform this guard early.src/index.ts (1)
69-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
CONFIGbefore the first dereference.CI is currently crashing here with
TypeError: undefined is not an object (evaluating 'CONFIG.webServerEnabled'), so the plugin never finishes bootstrapping. Please fail fast ifinitConfig(directory)does not populateCONFIG, or switch this file to a getter/local config object instead of dereferencing the mutable global immediately.🤖 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/index.ts` around lines 69 - 75, The code dereferences the global CONFIG before ensuring it was initialized, causing a crash; update startup in the module that calls startWebServer to guard CONFIG (the global) or replace direct global access with a local config value returned from initConfig(directory) and check it for truthiness before using it. Specifically, ensure initConfig(...) is awaited and its result is validated (or throw a clear error) before evaluating CONFIG.webServerEnabled, or obtain a local const config = getConfig()/initConfig(...) and use config.webServerEnabled, config.webServerPort, config.webServerHost, and config.webServerApiKey when calling startWebServer; fail fast with a descriptive error if the config is missing.
🤖 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/api-handlers.ts`:
- Around line 512-520: The cross-shard pagination is undercounting because you
only fetch pageSize results per shard before doing a global sort/slice; in the
block using shardManager.getAllShards(...) and calling
vectorSearch.searchInShard(shard, queryVector, "", pageSize), change the
per-shard fetch limit to at least page * pageSize (e.g., compute const
perShardLimit = page * pageSize and call vectorSearch.searchInShard(shard,
queryVector, "", perShardLimit)) so each shard can contribute enough candidates
for correct global ranking and totalPages calculation; keep the existing dedupe
by shard.dbPath and existing global sort/slice logic but use the larger
per-shard result sets (you may optionally cap perShardLimit to a sensible max to
avoid excessive work).
In `@src/services/web-server-worker.ts`:
- Around line 94-105: The current auth gate (requiresAuth computed from
workerConfig + isLocalhost) runs for all requests and checks
req.headers.get("x-opencode-mem-key"), which causes index.html/app.js to return
401 for browser loads; change the check so it only enforces the header for API
endpoints (not static app serving). Concretely, in the request handler around
the requiresAuth logic, detect API routes (e.g., request path prefix like "/api"
or requests that expect JSON) and only perform the apiKey header check and
jsonResponse(…,401) for those paths; allow requests for index.html, app.js and
other static assets to bypass this header check so the browser UI can load and
then send the key on subsequent API calls. Ensure you still use workerConfig,
isLocalhost, and req.headers.get("x-opencode-mem-key") for the guarded API
branch so behavior is unchanged for non-localhost API requests.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 69-75: The code dereferences the global CONFIG before ensuring it
was initialized, causing a crash; update startup in the module that calls
startWebServer to guard CONFIG (the global) or replace direct global access with
a local config value returned from initConfig(directory) and check it for
truthiness before using it. Specifically, ensure initConfig(...) is awaited and
its result is validated (or throw a clear error) before evaluating
CONFIG.webServerEnabled, or obtain a local const config =
getConfig()/initConfig(...) and use config.webServerEnabled,
config.webServerPort, config.webServerHost, and config.webServerApiKey when
calling startWebServer; fail fast with a descriptive error if the config is
missing.
In `@src/services/web-server-worker.ts`:
- Around line 370-380: Before calling serve() and binding the socket, validate
the requested host/port and API key: if message.apiKey is missing/empty and the
requested hostname (message.host) is not a localhost address (e.g., not
"localhost", "127.0.0.1", "::1"), reject the start request and do not assign
workerConfig or call serve(); surface a clear error (throw or return) so server
is never created. Update the logic around workerConfig, message.apiKey, and the
call to serve() (and any code that sets server) to perform this guard early.
🪄 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 Plus
Run ID: 22459a14-e908-496c-9181-e58ae0b21f02
📒 Files selected for processing (82)
scripts/build.mjsscripts/migrate-tests.mjsscripts/migrate-v1-to-v2.tssrc/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/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/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/vector-backends/backend-factory.tssrc/services/vector-backends/exact-scan-backend.tssrc/services/vector-backends/types.tssrc/services/vector-backends/usearch-backend.tssrc/services/web-server-worker.tssrc/services/web-server.tssrc/types/index.tssrc/types/usearch.d.tssrc/web/app.jssrc/web/i18n.jstests/ai-provider-config.test.tstests/anthropic-provider.test.tstests/config-resolution.test.tstests/config.test.tstests/language-detector.test.tstests/memory-engine.test.tstests/memory-scope.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/tags.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/windows-path.test.ts
💤 Files with no reviewable changes (78)
- src/services/ai/providers/openai-chat-completion.ts
- tests/profile-tool-runtime.test.ts
- src/services/user-profile/user-profile-manager.ts
- src/web/i18n.js
- src/services/platform-server.ts
- src/services/ai/providers/base-provider.ts
- tests/vector-search-backend-integration.test.ts
- src/services/migration-service.ts
- src/services/sqlite/transcript-manager.ts
- src/services/ai/providers/openai-responses.ts
- tests/config-resolution.test.ts
- src/types/index.ts
- tests/windows-path.test.ts
- src/services/user-profile/profile-utils.ts
- src/services/sqlite/connection-manager.ts
- src/services/cleanup-service.ts
- src/services/memory-conflicts.ts
- src/services/embedding.ts
- src/services/user-profile/types.ts
- tests/config.test.ts
- tests/privacy.test.ts
- src/services/transcript-capture.ts
- src/services/deduplication-service.ts
- tests/language-detector.test.ts
- src/services/ai/session/session-types.ts
- src/config.ts
- src/services/jsonc.ts
- src/services/sqlite/vector-search.ts
- tests/memory-scope.test.ts
- tests/ai-provider-config.test.ts
- src/services/ai/ai-provider-factory.ts
- src/types/usearch.d.ts
- src/services/vector-backends/usearch-backend.ts
- src/services/tags.ts
- src/services/privacy.ts
- src/services/user-profile/profile-context.ts
- src/services/sqlite/sqlite-bootstrap.ts
- src/plugin.ts
- src/services/ai/providers/google-gemini.ts
- tests/profile-write.test.ts
- tests/tool-scope.test.ts
- src/services/memory-lifecycle.ts
- tests/memory-engine.test.ts
- tests/project-scope.test.ts
- src/services/sqlite/types.ts
- src/services/user-prompt/user-prompt-manager.ts
- tests/vector-backends/usearch-backend.test.ts
- src/services/secret-resolver.ts
- src/services/client.ts
- src/services/ai/validators/user-profile-validator.ts
- tests/anthropic-provider.test.ts
- src/services/retrieval-context.ts
- src/services/vector-backends/types.ts
- src/services/ai/opencode-provider.ts
- src/services/web-server.ts
- tests/opencode-provider.test.ts
- scripts/build.mjs
- src/services/sqlite/shard-manager.ts
- src/services/memory-scoring-service.ts
- src/services/ai/providers/anthropic-messages.ts
- tests/vector-backends/exact-scan-backend.test.ts
- tests/openai-chat-completion-provider.test.ts
- src/services/ai/session/ai-session-manager.ts
- tests/tags.test.ts
- src/services/ai/tools/tool-schema.ts
- tests/plugin-loader-contract.test.ts
- tests/vector-backends/migration-fallback.test.ts
- tests/vector-backends/backend-factory.test.ts
- src/services/user-memory-learning.ts
- scripts/migrate-v1-to-v2.ts
- src/services/memory-scoring.ts
- src/services/context.ts
- src/services/vector-backends/exact-scan-backend.ts
- src/services/auto-capture.ts
- src/services/ai/provider-config.ts
- src/services/language-detector.ts
- src/services/vector-backends/backend-factory.ts
- scripts/migrate-tests.mjs
- web-server-worker.ts: Fix auth bypass when enabled is undefined — remove enabled from requiresAuth gate, rely only on apiKey + non-localhost host - web-server-worker.ts: Narrow PII redaction regex to exact key matches (apiKey|access_token|refresh_token|secret|password|token|authorization) to prevent false positives like accessCount / lastAccessed - web-server-worker.ts: Restrict API key auth to /api/* paths only, allowing browser to load static assets (index.html, app.js, styles.css) without the x-opencode-mem-key header - api-handlers.ts: Fix cross-shard pagination truncation — fetch page*pageSize (capped at 500) per shard instead of just pageSize, ensuring correct global ranking for page > 1
Owner
Author
|
@coderabbitai Please perform a comprehensive full-repo review on the latest state of this branch. All previous findings have been addressed. Review the entire codebase for any remaining issues — security, type safety, error handling, resource leaks, logic bugs, race conditions, cross-platform issues, test coverage, and performance. This is the final verification pass before merge. |
|
Tip For best results, initiate chat on the files or code changes.
Results will be posted as inline review comments momentarily. [full_review] |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api-handlers.ts`:
- Around line 519-520: Validate and clamp the incoming page and pageSize values
before using them to compute per-shard limits: ensure page and pageSize are
numbers >= 1 (and pageSize capped to a reasonable max, e.g., 500), compute a
sanitizedLimit = Math.min(clampedPage * clampedPageSize, 500) once, reuse
sanitizedLimit instead of recalculating perShardLimit, and pass that
sanitizedLimit into vectorSearch.searchInShard (and any other places in this
handler) to avoid NaN/negative shard limits and silent partial results; update
references to page, pageSize, perShardLimit and vectorSearch.searchInShard
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 Plus
Run ID: 3ec26e0c-1781-46bf-9d4a-ede2c68d1aa0
📒 Files selected for processing (2)
src/services/api-handlers.tssrc/services/web-server-worker.ts
…aining issues - api-handlers.ts: Add safePage/safePageSize sanitization in handleSearch and handleListMemories to prevent NaN/negative/invalid pagination inputs - web-server.ts: Sanitize page/pageSize query params before passing to handlers - web-server-worker.ts: Sanitize page/pageSize query params before passing to handlers - All pagination now uses Number.isFinite checks and caps pageSize at 100
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 (2)
src/services/api-handlers.ts (2)
645-645:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResponse returns raw
page/pageSizeinstead of sanitized values.Same issue as in
handleListMemories- the response returns original parameters rather than the sanitizedsafePage/safePageSizevalues, causing a mismatch withtotalPages.Proposed fix
- return { success: true, data: { items: paginatedResults, total, page, pageSize, totalPages } }; + return { success: true, data: { items: paginatedResults, total, page: safePage, pageSize: safePageSize, totalPages } };🤖 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/api-handlers.ts` at line 645, The response currently returns the original page and pageSize parameters, causing a mismatch with the sanitized pagination values; update the handler that returns { success: true, data: { items: paginatedResults, total, page, pageSize, totalPages } } to use the sanitized safePage and safePageSize instead of page/pageSize (same fix as in handleListMemories), so the payload becomes items: paginatedResults, total, page: safePage, pageSize: safePageSize, totalPages and ensure totalPages is computed based on those sanitized values.
276-276:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResponse returns raw
page/pageSizeinstead of sanitized values.The response at line 276 returns the original
pageandpageSizeparameters rather thansafePageandsafePageSize. This creates a mismatch wheretotalPagesis calculated fromsafePageSizebut the returnedpageSizemight be the unsanitized value (e.g., negative or NaN), confusing API consumers.Proposed fix
- return { success: true, data: { items, total, page, pageSize, totalPages } }; + return { success: true, data: { items, total, page: safePage, pageSize: safePageSize, totalPages } };🤖 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/api-handlers.ts` at line 276, The return payload is using the original page and pageSize values causing inconsistency with totalPages; update the response to return the sanitized pagination values (safePage and safePageSize) instead of page and pageSize so totalPages (computed from safePageSize) matches the returned pageSize and the API never exposes unsanitized/invalid pagination values; adjust the return object that currently contains { items, total, page, pageSize, totalPages } to use safePage and safePageSize while keeping items, total, and totalPages as-is.
♻️ Duplicate comments (1)
src/services/api-handlers.ts (1)
537-537:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBug: Uses raw
pageSizeinstead ofsafePageSizefor prompt search in non-tag path.Line 537 uses the unsanitized
pageSizeparameter instead ofsafePageSize, which could result inNaN * 2or negative values being passed tosearchPrompts. This is inconsistent with the tag-based path at line 521 which correctly usessafePageSize.Proposed fix
- promptResults = userPromptManager.searchPrompts(query, undefined, pageSize * 2); + promptResults = userPromptManager.searchPrompts(query, undefined, safePageSize * 2);🤖 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/api-handlers.ts` at line 537, The non-tag branch incorrectly calls userPromptManager.searchPrompts(query, undefined, pageSize * 2) using the raw pageSize; change this to use the sanitized safePageSize (i.e., call userPromptManager.searchPrompts(query, undefined, safePageSize * 2)) so the searchPrompts invocation uses the validated value used in the tag path and avoids NaN/negative inputs; ensure safePageSize is the same variable computed earlier in this handler before replacing the call.
🧹 Nitpick comments (1)
src/services/web-server-worker.ts (1)
348-356: 💤 Low valueRedacting all API responses may unexpectedly strip legitimate data.
Applying
redactPIIunconditionally to all JSON responses could cause issues:
- The email regex (
[a-zA-Z0-9._%+-]+@...) may match non-email strings (e.g., version strings likelib-1.0+build@v2.com)- Local/development clients may need unredacted data for debugging
Consider making redaction conditional based on
isLocalhostsimilar to howweb-server.tshandles it with theredactparameter.🤖 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/web-server-worker.ts` around lines 348 - 356, The jsonResponse function currently unconditionally calls redactPII which can strip valid data; change jsonResponse to accept a redact flag (or determine redaction from isLocalhost like web-server.ts) and only call redactPII(data) when redaction is enabled (e.g., redact === true && !isLocalhost). Update all call sites that use jsonResponse to pass the appropriate redact value (default true for production routes, false for local/dev debug paths) and preserve the existing JSON.stringify/Response behavior when skipping redaction; reference jsonResponse and redactPII to locate the change and mirror the conditional pattern used in web-server.ts.
🤖 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.
Outside diff comments:
In `@src/services/api-handlers.ts`:
- Line 645: The response currently returns the original page and pageSize
parameters, causing a mismatch with the sanitized pagination values; update the
handler that returns { success: true, data: { items: paginatedResults, total,
page, pageSize, totalPages } } to use the sanitized safePage and safePageSize
instead of page/pageSize (same fix as in handleListMemories), so the payload
becomes items: paginatedResults, total, page: safePage, pageSize: safePageSize,
totalPages and ensure totalPages is computed based on those sanitized values.
- Line 276: The return payload is using the original page and pageSize values
causing inconsistency with totalPages; update the response to return the
sanitized pagination values (safePage and safePageSize) instead of page and
pageSize so totalPages (computed from safePageSize) matches the returned
pageSize and the API never exposes unsanitized/invalid pagination values; adjust
the return object that currently contains { items, total, page, pageSize,
totalPages } to use safePage and safePageSize while keeping items, total, and
totalPages as-is.
---
Duplicate comments:
In `@src/services/api-handlers.ts`:
- Line 537: The non-tag branch incorrectly calls
userPromptManager.searchPrompts(query, undefined, pageSize * 2) using the raw
pageSize; change this to use the sanitized safePageSize (i.e., call
userPromptManager.searchPrompts(query, undefined, safePageSize * 2)) so the
searchPrompts invocation uses the validated value used in the tag path and
avoids NaN/negative inputs; ensure safePageSize is the same variable computed
earlier in this handler before replacing the call.
---
Nitpick comments:
In `@src/services/web-server-worker.ts`:
- Around line 348-356: The jsonResponse function currently unconditionally calls
redactPII which can strip valid data; change jsonResponse to accept a redact
flag (or determine redaction from isLocalhost like web-server.ts) and only call
redactPII(data) when redaction is enabled (e.g., redact === true &&
!isLocalhost). Update all call sites that use jsonResponse to pass the
appropriate redact value (default true for production routes, false for
local/dev debug paths) and preserve the existing JSON.stringify/Response
behavior when skipping redaction; reference jsonResponse and redactPII to locate
the change and mirror the conditional pattern used in web-server.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5447beb-89c9-47a2-ac47-a63e8b8a7367
📒 Files selected for processing (3)
src/services/api-handlers.tssrc/services/web-server-worker.tssrc/services/web-server.ts
…ponses - handleSearch: return safePage/safePageSize in response instead of raw params - handleListMemories: return safePage/safePageSize in response instead of raw params - Prevents API consumers from receiving NaN/negative pagination values
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/services/api-handlers.ts (1)
540-540:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBug: Raw
pageSizeused instead ofsafePageSize.Line 540 uses the raw
pageSizeparameter instead of the sanitizedsafePageSize, defeating the validation performed at lines 500-502. This can propagate NaN, negative, or unbounded values tosearchPrompts.🐛 Proposed fix
- promptResults = userPromptManager.searchPrompts(query, undefined, pageSize * 2); + promptResults = userPromptManager.searchPrompts(query, undefined, safePageSize * 2);🤖 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/api-handlers.ts` at line 540, The call to userPromptManager.searchPrompts uses the raw pageSize variable (promptResults = userPromptManager.searchPrompts(query, undefined, pageSize * 2)) which bypasses the earlier validation; change it to use the sanitized safePageSize (i.e. pass safePageSize * 2) so only validated numeric bounds are forwarded to searchPrompts, ensuring safePageSize is defined and numeric before multiplication.
🧹 Nitpick comments (1)
src/services/api-handlers.ts (1)
512-517: ⚡ Quick winConsider using page-aware limit for tag-based search.
Unlike the non-tag search (line 533) which uses
safePage * safePageSizeto ensure sufficient candidates for later pages, the tag-based search uses a fixedsafePageSize * 2regardless of page number. Forpage > 2, this could truncate results similarly to the issue previously fixed for non-tag searches.♻️ Suggested change for consistency
- const results = await vectorSearch.searchInShard( - shard, - queryVector, - tag, - safePageSize * 2 - ); + const perShardLimit = Math.min(safePage * safePageSize, 500); + const results = await vectorSearch.searchInShard( + shard, + queryVector, + tag, + perShardLimit + );🤖 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/api-handlers.ts` around lines 512 - 517, The tag-based shard search currently requests a fixed number of candidates using safePageSize * 2 which will underfetch for page > 2; update the call to vectorSearch.searchInShard in the tag branch to request page-aware limits (use safePage * safePageSize or otherwise ensure at least safePage * safePageSize candidates, e.g. Math.max(safePage * safePageSize, safePageSize * 2)) so it matches the non-tag behavior and prevents truncation of later pages.
🤖 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.
Duplicate comments:
In `@src/services/api-handlers.ts`:
- Line 540: The call to userPromptManager.searchPrompts uses the raw pageSize
variable (promptResults = userPromptManager.searchPrompts(query, undefined,
pageSize * 2)) which bypasses the earlier validation; change it to use the
sanitized safePageSize (i.e. pass safePageSize * 2) so only validated numeric
bounds are forwarded to searchPrompts, ensuring safePageSize is defined and
numeric before multiplication.
---
Nitpick comments:
In `@src/services/api-handlers.ts`:
- Around line 512-517: The tag-based shard search currently requests a fixed
number of candidates using safePageSize * 2 which will underfetch for page > 2;
update the call to vectorSearch.searchInShard in the tag branch to request
page-aware limits (use safePage * safePageSize or otherwise ensure at least
safePage * safePageSize candidates, e.g. Math.max(safePage * safePageSize,
safePageSize * 2)) so it matches the non-tag behavior and prevents truncation of
later pages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b7682c8-9696-47eb-baa6-7bbfd402fe71
📒 Files selected for processing (1)
src/services/api-handlers.ts
- handleSearch: use safePageSize instead of raw pageSize for promptResults limit - Prevents invalid/NaN limits being passed to userPromptManager.searchPrompts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/api-handlers.ts (1)
512-517:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLater search pages still undercount because these fetch limits ignore
page.These branches only load
safePageSize * 2candidates before the global sort/slice. Forpage > 2, matching tag-scoped memories and prompts can never participate in ranking, soitemsandtotalPagesdrift from the real result set. Use a window derived fromsafePage * safePageSizefor every source that feedscombinedResults.Also applies to: 524-524, 540-540
🤖 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/api-handlers.ts` around lines 512 - 517, The shard search fetch limit currently uses safePageSize * 2 which prevents results beyond page 2 from ever being included; change calls to vectorSearch.searchInShard (and the similar calls that feed combinedResults) to request a window based on safePage * safePageSize (i.e., safePage multiplied by safePageSize) so each source returns enough candidates for global sort/slice for the requested page; update the three call sites (the vectorSearch.searchInShard invocation and the two other similar fetches that feed combinedResults) to use this computed window variable and keep the existing global sort/slice logic unchanged.
🤖 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/api-handlers.ts`:
- Around line 145-147: The code caps safePageSize but not safePage, so a huge
page value can be passed into listMemories and cause an effectively unbounded
read; modify the request handling that computes safePage to constrain the
overall window (e.g., enforce a maxPage and/or maxOffset = maxPage *
safePageSize) or reject requests where page > MAX_PAGE before calling
listMemories; update the logic around safePage/safePageSize and the call to
listMemories to use the bounded page or computed maxOffset (reference safePage,
safePageSize, listMemories) so storage is never asked to read beyond the allowed
window.
---
Duplicate comments:
In `@src/services/api-handlers.ts`:
- Around line 512-517: The shard search fetch limit currently uses safePageSize
* 2 which prevents results beyond page 2 from ever being included; change calls
to vectorSearch.searchInShard (and the similar calls that feed combinedResults)
to request a window based on safePage * safePageSize (i.e., safePage multiplied
by safePageSize) so each source returns enough candidates for global sort/slice
for the requested page; update the three call sites (the
vectorSearch.searchInShard invocation and the two other similar fetches that
feed combinedResults) to use this computed window variable and keep the existing
global sort/slice logic unchanged.
🪄 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 Plus
Run ID: 1c0c9203-ea4e-4870-b8ba-5e77fe047582
📒 Files selected for processing (1)
src/services/api-handlers.ts
Comment on lines
+145
to
+147
| const safePage = Number.isFinite(page) && page > 0 ? Math.floor(page) : 1; | ||
| const safePageSize = | ||
| Number.isFinite(pageSize) && pageSize > 0 && pageSize <= 100 ? Math.floor(pageSize) : 20; |
There was a problem hiding this comment.
Bound the page window before using it as a shard read limit.
safePageSize is capped, but safePage is not. On Line 153, a very large page can still turn listMemories into an effectively unbounded read and materialize far more rows than this endpoint can safely return. Cap or reject oversized page windows before calling storage.
Suggested fix
try {
const safePage = Number.isFinite(page) && page > 0 ? Math.floor(page) : 1;
const safePageSize =
Number.isFinite(pageSize) && pageSize > 0 && pageSize <= 100 ? Math.floor(pageSize) : 20;
+ const requestedWindow = safePage * safePageSize;
+ if (requestedWindow > 500) {
+ return { success: false, error: "Requested page window is too large" };
+ }
await embeddingService.warmup();
let allMemories: any[] = [];
if (tag) {
const { scope: tagScope, hash } = extractScopeFromTag(tag);
const shards = shardManager.getAllShards(tagScope, hash);
- const limit = safePage * safePageSize; // Fetch enough to cover the requested page across shards
+ const limit = requestedWindow;Also applies to: 153-153
🤖 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/api-handlers.ts` around lines 145 - 147, The code caps
safePageSize but not safePage, so a huge page value can be passed into
listMemories and cause an effectively unbounded read; modify the request
handling that computes safePage to constrain the overall window (e.g., enforce a
maxPage and/or maxOffset = maxPage * safePageSize) or reject requests where page
> MAX_PAGE before calling listMemories; update the logic around
safePage/safePageSize and the call to listMemories to use the bounded page or
computed maxOffset (reference safePage, safePageSize, listMemories) so storage
is never asked to read beyond the allowed window.
…d limit computation - Limit page number to 10,000 max in api-handlers, web-server, and web-server-worker - Addresses CodeRabbit finding about very large page numbers causing unbounded limit read in vectorSearch.listMemories
ZeR020
changed the title
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.
Purpose
This PR is opened solely to trigger a comprehensive CodeRabbit full-repo audit of all fixes and changes accumulated across previous audit rounds.
Scope
Please review the entire repository (all tracked files) for:
Context
This branch contains:
Note
DO NOT MERGE — this PR exists only for the automated review. All verified fixes will be merged via separate PR.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements