Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR moves recall feedback to runtime app-config control, updates related control-portal, tool, MCP, and UI wiring, revises recall telemetry queries, refactors Neo4j read paths, and includes web loading, schema, and maintenance updates. ChangesRecall feedback runtime configuration
Sparse telemetry query updates
Backend data-path and request handling
Web request and loading infrastructure
Platform maintenance and cleanup
Sequence Diagram(s)sequenceDiagram
participant ControlPortal
participant AppConfigService
participant ToolExecuteHandler
participant MCPServer
ControlPortal->>AppConfigService: UpdateRecallFeedbackSettings(values)
AppConfigService-->>ControlPortal: effective.enabled
ToolExecuteHandler->>AppConfigService: RecallFeedbackRuntimeConfig(ctx)
AppConfigService-->>ToolExecuteHandler: enabled/disabled
MCPServer->>AppConfigService: RecallFeedbackRuntimeConfig(ctx)
AppConfigService-->>MCPServer: enabled/disabled
sequenceDiagram
participant HostLLM
participant MCPServer
participant RecallTool as recall_memory
participant FeedbackTool as submit_recall_session_feedback
HostLLM->>MCPServer: tools/call recall_memory
MCPServer->>RecallTool: invoke
RecallTool-->>MCPServer: recall_event
HostLLM->>MCPServer: tools/call submit_recall_session_feedback
MCPServer->>FeedbackTool: invoke
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Suggested labels
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 |
![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
🧹 Nitpick comments (5)
internal/tools/registry/toolset.go (1)
347-352: ⚡ Quick winUse the shared tool-name constant in
feedback_request.
feedback_request.toolis hardcoded to"submit_recall_feedback"while the tool registration usesSubmitRecallFeedbackToolName. Reuse the constant to avoid drift between advertised and registered names.Proposed diff
- out["feedback_request"] = map[string]any{ + out["feedback_request"] = map[string]any{ "requested": true, "recall_id": "rec_" + uuid.NewString(), - "tool": "submit_recall_feedback", + "tool": SubmitRecallFeedbackToolName, "fields": []string{🤖 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 `@internal/tools/registry/toolset.go` around lines 347 - 352, The hardcoded string "submit_recall_feedback" in the feedback_request map's "tool" field should be replaced with the constant SubmitRecallFeedbackToolName to maintain consistency with the tool registration and prevent drift. In the RecallFeedbackEnabled conditional block where out["feedback_request"] is being constructed, change the "tool" field value from the hardcoded string literal to reference the SubmitRecallFeedbackToolName constant instead.internal/tools/registry/recall_feedback_test.go (1)
66-67: ⚡ Quick winAssert the sentinel error instead of matching error text.
Using
strings.Contains(err.Error(), "tool disabled")is brittle. Prefererrors.Is(err, ErrToolDisabled)to lock this test to behavior rather than wording.Proposed diff
import ( "context" + "errors" "strings" "testing" @@ - if err == nil || !strings.Contains(err.Error(), "tool disabled") { + if err == nil || !errors.Is(err, ErrToolDisabled) { t.Fatalf("err = %v; want disabled tool", err) }🤖 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 `@internal/tools/registry/recall_feedback_test.go` around lines 66 - 67, The error assertion in the test at lines 66-67 uses brittle string matching with strings.Contains to check for "tool disabled" in the error message, which is fragile and dependent on exact wording. Replace the string comparison logic by using errors.Is to check against the ErrToolDisabled sentinel error instead. Update the condition to use errors.Is(err, ErrToolDisabled) to make the test more robust and behavior-focused rather than text-focused.internal/http/control_portal_config_test.go (1)
168-179: ⚡ Quick winStrengthen PATCH success assertion to validate returned state.
The flow currently proves request forwarding (
appConfig.recallValues) but not that the response reflects the updated runtime state. Updating the stub to mutaterecallSettings/recallRuntimeand asserting"enabled":trueafter PATCH will catch stale-response regressions.Suggested test/stub refinement
import ( "context" "errors" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" @@ func (s *controlAppConfigSvc) UpdateRecallFeedbackSettings(_ context.Context, values map[string]string, _, _, _ string) (*domain.RecallFeedbackConfigSettings, error) { if s.updateErr != nil { return nil, s.updateErr } if s.recallValues == nil { s.recallValues = make(map[string]string) } for key, value := range values { s.recallValues[key] = value } + if raw, ok := values[domain.AppConfigRecallFeedbackEnabled]; ok { + enabled, _ := strconv.ParseBool(raw) + s.recallRuntime.Enabled = enabled + if s.recallSettings != nil { + s.recallSettings.Effective.Enabled = enabled + for i := range s.recallSettings.Items { + if s.recallSettings.Items[i].Key == domain.AppConfigRecallFeedbackEnabled { + s.recallSettings.Items[i].Value = raw + s.recallSettings.Items[i].EffectiveValue = strconv.FormatBool(enabled) + } + } + } + } return s.recallSettings, nil } @@ rec = do(http.MethodPatch, "/control/api/config/recall-feedback", `{"items":[{"key":"RECALL_FEEDBACK_ENABLED","value":"true"}]}`) require.Equal(t, http.StatusOK, rec.Code) require.Equal(t, "true", appConfig.recallValues[domain.AppConfigRecallFeedbackEnabled]) + require.Contains(t, rec.Body.String(), `"enabled":true`)Also applies to: 451-496
🤖 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 `@internal/http/control_portal_config_test.go` around lines 168 - 179, The stub method UpdateRecallFeedbackSettings accepts input values and stores them in recallValues, but does not update the recallSettings that gets returned to the caller, so the test only validates request forwarding but not response state. Modify the UpdateRecallFeedbackSettings stub to mutate s.recallSettings with the provided values before returning it, then update the corresponding test assertion to verify that the returned response contains the expected "enabled":true state, which will catch regressions where stale responses are incorrectly returned.internal/http/handler/tool_catalog_handler_test.go (1)
170-176: ⚡ Quick winAssert success status before checking catalog contents.
This test currently passes on body substring alone; a non-200 response could still pass if it omits the tool name. Add an explicit
StatusOKassertion first.Suggested patch
req := httptest.NewRequest(http.MethodGet, "/api/v1/tools", nil) rec := httptest.NewRecorder() e.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d; want 200. body=%s", rec.Code, rec.Body.String()) + } if strings.Contains(rec.Body.String(), registry.SubmitRecallFeedbackToolName) { t.Fatalf("disabled recall feedback tool leaked into catalog: %s", rec.Body.String()) }🤖 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 `@internal/http/handler/tool_catalog_handler_test.go` around lines 170 - 176, The test does not verify that the HTTP response has a successful status code before checking the response body. Add an explicit assertion to check that rec.StatusCode equals http.StatusOK immediately after the e.ServeHTTP call and before the strings.Contains check on rec.Body.String(). This ensures the test properly validates both a successful response and that the disabled tool does not appear in the catalog contents.internal/mcp/server_test.go (1)
255-258: ⚡ Quick winPrefer structured
tools/listassertions over raw substring matching.Checking raw JSON text is brittle; assert against decoded
toolsentries to validate absence deterministically.Suggested patch
out := runRPC(t, s, `{"jsonrpc":"2.0","id":2,"method":"tools/list"}`) - if strings.Contains(out, registry.SubmitRecallFeedbackToolName) { - t.Fatalf("tools/list exposed disabled recall feedback tool: %s", out) - } + var listResp rpcResp + if err := json.Unmarshal([]byte(strings.TrimSpace(out)), &listResp); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if listResp.Error != nil { + t.Fatalf("tools/list error = %+v", listResp.Error) + } + var payload struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + } + if err := json.Unmarshal(listResp.Result, &payload); err != nil { + t.Fatalf("result unmarshal: %v", err) + } + for _, tool := range payload.Tools { + if tool.Name == registry.SubmitRecallFeedbackToolName { + t.Fatalf("tools/list exposed disabled recall feedback tool: %s", out) + } + }🤖 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 `@internal/mcp/server_test.go` around lines 255 - 258, Replace the raw substring matching using strings.Contains on the output with structured JSON decoding and assertion. Parse the response from runRPC into a structured format (likely containing a tools array), then iterate through or check the decoded tools entries to verify that registry.SubmitRecallFeedbackToolName is not present in the actual tools list rather than performing a brittle string search on the raw JSON output.
🤖 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 `@internal/mcp/server.go`:
- Around line 212-214: The error handling at Lines 233-240 during tool
invocation does not properly map the registry.ErrToolDisabled error to
errCodeMethodNotFound, which breaks consistency with the visibility check at
Line 212. In the invoke error mapping section (Lines 233-240), add specific
handling to catch registry.ErrToolDisabled errors and return an rpcError with
errCodeMethodNotFound and an appropriate message like "tool not found:
{toolName}" to maintain the same hidden-tool contract applied by the ToolVisible
check.
In `@web/tests-compose/compose-portal.spec.ts`:
- Around line 130-132: The test "MCP recall feedback is submitted and scraped
through compose telemetry" calls enableRecallFeedback to enable a global runtime
flag but does not reset it after the test completes, causing state leakage to
subsequent tests. Add a finally block or afterEach hook following the
enableRecallFeedback call to reset the recall feedback flag to its original
state, ensuring the test suite remains deterministic and test execution order
does not cause unexpected behavior.
---
Nitpick comments:
In `@internal/http/control_portal_config_test.go`:
- Around line 168-179: The stub method UpdateRecallFeedbackSettings accepts
input values and stores them in recallValues, but does not update the
recallSettings that gets returned to the caller, so the test only validates
request forwarding but not response state. Modify the
UpdateRecallFeedbackSettings stub to mutate s.recallSettings with the provided
values before returning it, then update the corresponding test assertion to
verify that the returned response contains the expected "enabled":true state,
which will catch regressions where stale responses are incorrectly returned.
In `@internal/http/handler/tool_catalog_handler_test.go`:
- Around line 170-176: The test does not verify that the HTTP response has a
successful status code before checking the response body. Add an explicit
assertion to check that rec.StatusCode equals http.StatusOK immediately after
the e.ServeHTTP call and before the strings.Contains check on rec.Body.String().
This ensures the test properly validates both a successful response and that the
disabled tool does not appear in the catalog contents.
In `@internal/mcp/server_test.go`:
- Around line 255-258: Replace the raw substring matching using strings.Contains
on the output with structured JSON decoding and assertion. Parse the response
from runRPC into a structured format (likely containing a tools array), then
iterate through or check the decoded tools entries to verify that
registry.SubmitRecallFeedbackToolName is not present in the actual tools list
rather than performing a brittle string search on the raw JSON output.
In `@internal/tools/registry/recall_feedback_test.go`:
- Around line 66-67: The error assertion in the test at lines 66-67 uses brittle
string matching with strings.Contains to check for "tool disabled" in the error
message, which is fragile and dependent on exact wording. Replace the string
comparison logic by using errors.Is to check against the ErrToolDisabled
sentinel error instead. Update the condition to use errors.Is(err,
ErrToolDisabled) to make the test more robust and behavior-focused rather than
text-focused.
In `@internal/tools/registry/toolset.go`:
- Around line 347-352: The hardcoded string "submit_recall_feedback" in the
feedback_request map's "tool" field should be replaced with the constant
SubmitRecallFeedbackToolName to maintain consistency with the tool registration
and prevent drift. In the RecallFeedbackEnabled conditional block where
out["feedback_request"] is being constructed, change the "tool" field value from
the hardcoded string literal to reference the SubmitRecallFeedbackToolName
constant instead.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 0d8e9e49-eb7d-41cd-b098-5b06ce4cc2b9
📒 Files selected for processing (31)
README.mdREADME.zh-CN.mdcmd/demo-server/main.gocmd/server/main.gointernal/config/config.gointernal/config/config_test.gointernal/domain/app_config.gointernal/http/control_portal.gointernal/http/control_portal_config.gointernal/http/control_portal_config_test.gointernal/http/handler/mcp_handler.gointernal/http/handler/tool_catalog_handler.gointernal/http/handler/tool_catalog_handler_test.gointernal/http/handler/tool_execute_handler.gointernal/http/handler/tool_execute_handler_test.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/service/app_config_payload.gointernal/service/app_config_recall_feedback.gointernal/service/app_config_service.gointernal/service/app_config_service_test.gointernal/tools/registry/recall_feedback_test.gointernal/tools/registry/runtime_config.gointernal/tools/registry/toolset.gointernal/tools/registry/toolset_test.gomigrations/postgres/2026061624_recall_feedback_config.sqlscripts/e2e-compose.shweb/src/api.tsweb/src/control/ConfigPanel.tsxweb/tests-compose/compose-portal.spec.tsweb/tests/control-portal.spec.ts
💤 Files with no reviewable changes (3)
- scripts/e2e-compose.sh
- internal/config/config_test.go
- internal/config/config.go
gitvibe-for-github
Bot
added
the
gvi:reviewing
![gitvibe-for-github[bot]](https://avatars.githubusercontent.com/in/4002928?s=60&v=4){kind=small}
There was a problem hiding this comment.
GitVibe Review Matrix
Status: completed
Next state: changes-required
Review synthesis completed for PR #37 at 80d48f8. The runtime app_config implementation is mostly consistent with the requested no-env-fallback behavior, but two evidence-backed fixes are required before approval: MCP disabled-tool errors need the same hidden-tool mapping as discovery, and the compose e2e test must restore the global recall-feedback flag it enables.
Next Action
Continue with changes-required.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27658071297
Inline comments: 2
Required Fixes
- internal/mcp/server.go:212 establishes a hidden-tool visibility check for disabled recall feedback, but internal/mcp/server.go:233-240 maps any later tool.Invoke error to errCodeToolFailure. submit_recall_feedback's Invoke returns registry.ErrToolDisabled when runtime config is disabled, so a config change or second runtime check can return -32000 instead of method-not-found, breaking the hidden-tool contract for MCP callers.
- web/tests-compose/compose-portal.spec.ts:131 enables RECALL_FEEDBACK_ENABLED via PATCH /control/api/config/recall-feedback and never restores it. Since app_config is durable process-global state, subsequent compose tests or reruns can become order-dependent and no longer start from the migration's disabled default.
gitvibe-for-github
Bot
added
gvi:blocked
gitvibe-for-github
Bot
added
gvi:reviewing
There was a problem hiding this comment.
GitVibe Review Matrix
Status: completed
Next state: review-passed
Review synthesis completed with 10/10 role results available. After inspecting the current PR head at 493ebd4, the previously reported blocking issues are resolved: MCP disabled-tool errors now map to method-not-found, the app_config migration sets transaction-local system RLS context, and recall runtime config uses the existing app config cache. No required fixes remain.
Next Action
Continue with review-passed.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27660024996
gitvibe-for-github
Bot
added
gvi:ready-for-approval
gitvibe-for-github
Bot
added
gvi:reviewing
There was a problem hiding this comment.
GitVibe Review Matrix
Status: blocked
Next state: blocked
No review-matrix matrix member results were available for synthesis. Expected 10.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27684778126
Inline comments: 0
Required Fixes
- No review-matrix matrix member results were available for synthesis. Expected 10.
gitvibe-for-github
Bot
added
gvi:blocked
gitvibe-for-github
Bot
added
gvi:reviewing
There was a problem hiding this comment.
GitVibe Review Matrix
Status: blocked
Next state: blocked
No review-matrix matrix member results were available for synthesis. Expected 10.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27686070968
Inline comments: 0
Required Fixes
- No review-matrix matrix member results were available for synthesis. Expected 10.
gitvibe-for-github
Bot
added
gvi:blocked
gitvibe-for-github
Bot
added
the
gvi:reviewing
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 (1)
internal/http/handler/tool_catalog_handler_test.go (1)
155-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis assertion contradicts the runtime-hide contract.
When recall feedback is disabled, discovery should not expose
submit_recall_session_feedback. This test currently enforces the opposite behavior.🔧 Proposed fix
- if !strings.Contains(rec.Body.String(), registry.SubmitRecallSessionFeedbackToolName) { - t.Fatalf("recall feedback tool missing from catalog: %s", rec.Body.String()) + if strings.Contains(rec.Body.String(), registry.SubmitRecallSessionFeedbackToolName) { + t.Fatalf("recall feedback tool must be hidden when disabled: %s", rec.Body.String()) }🤖 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 `@internal/http/handler/tool_catalog_handler_test.go` around lines 155 - 179, The test TestToolCatalogHandler_ListsRecallFeedbackToolWithRuntimeDisabled contradicts its own purpose by asserting that the submit_recall_session_feedback tool IS present in the response when the runtime is disabled. When recall feedback is disabled (enabled: false), the tool discovery should NOT expose the registry.SubmitRecallSessionFeedbackToolName tool. Fix this by inverting the assertion logic to verify that the tool is missing from the response body when disabled, rather than verifying it is present.
🧹 Nitpick comments (1)
internal/tools/registry/recall_feedback_test.go (1)
13-20: ⚡ Quick winAdd explicit fail-closed tests for config-provider errors.
The stub supports error returns, but there’s no test asserting provider errors disable recall feedback behavior (no
recall_event,ErrToolDisabledon submit). That’s a key runtime contract.✅ Suggested test addition
+func TestRecallFeedbackFailClosedOnConfigProviderError(t *testing.T) { + metrics := observability.NewInMemoryDiscoverabilityMetrics() + reg, _ := BuildDefault(Dependencies{ + Recall: stubRecallWithHit{}, + Metrics: metrics, + RecallFeedbackConfig: stubRecallFeedbackConfig{enabled: true, err: errors.New("config down")}, + }) + + recallTool, _ := reg.Get("recall_memory") + out, err := recallTool.Invoke(context.Background(), "profile-feedback", map[string]any{"query": "q"}) + if err != nil { + t.Fatalf("recall_memory Invoke: %v", err) + } + if _, ok := out["recall_event"]; ok { + t.Fatalf("recall_event must be absent when config provider errors") + } + + submitTool, _ := reg.Get("submit_recall_session_feedback") + _, err = submitTool.Invoke(context.Background(), "profile-feedback", map[string]any{ + "recalls": []any{map[string]any{ + "recall_id": "rec_1", "used": true, "answer_supported": true, "quality": "high", "missing_context": false, "irrelevant": false, + }}, + }) + if !errors.Is(err, ErrToolDisabled) { + t.Fatalf("err = %v; want ErrToolDisabled", err) + } +}🤖 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 `@internal/tools/registry/recall_feedback_test.go` around lines 13 - 20, Add explicit test cases to the recall_feedback_test.go file that verify the fail-closed behavior when the config provider returns an error. Create test scenarios where the stubRecallFeedbackConfig struct is initialized with a non-nil err value, and assert that when RecallFeedbackRuntimeConfig returns an error, the recall feedback system properly disables itself by ensuring no recall_event is generated and ErrToolDisabled is returned when attempting to submit feedback. This validates the runtime contract that configuration provider errors result in safe fail-closed behavior.
🤖 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 `@internal/http/handler/tool_execute_handler_test.go`:
- Around line 438-456: The test "runtime disabled tool descriptor is visible"
has inverted expectations for the disabled state. When the runtime config is
disabled (enabled: false), the GET request to retrieve the
submit_recall_session_feedback tool should return a not found status, not
success. Update the test to expect http.StatusNotFound instead of http.StatusOK,
and remove the assertion checking for the tool descriptor in the response body
(the require.Contains call). Also consider renaming the test to better reflect
what it's actually testing (that disabled tools return not found).
In `@internal/tools/registry/toolset.go`:
- Around line 346-351: The recall_event is being emitted whenever
RecallFeedbackEnabled returns true, but this creates a broken workflow when
deps.Metrics is nil because the submit_recall_session_feedback tool will fail.
Update the condition that guards the recall_event emission in the out map to
check both that RecallFeedbackEnabled(ctx, deps.RecallFeedbackConfig) is true
AND that deps.Metrics is not nil before assigning the recall_event map
containing recall_id, feedback_tool, and feedback_timing fields.
---
Outside diff comments:
In `@internal/http/handler/tool_catalog_handler_test.go`:
- Around line 155-179: The test
TestToolCatalogHandler_ListsRecallFeedbackToolWithRuntimeDisabled contradicts
its own purpose by asserting that the submit_recall_session_feedback tool IS
present in the response when the runtime is disabled. When recall feedback is
disabled (enabled: false), the tool discovery should NOT expose the
registry.SubmitRecallSessionFeedbackToolName tool. Fix this by inverting the
assertion logic to verify that the tool is missing from the response body when
disabled, rather than verifying it is present.
---
Nitpick comments:
In `@internal/tools/registry/recall_feedback_test.go`:
- Around line 13-20: Add explicit test cases to the recall_feedback_test.go file
that verify the fail-closed behavior when the config provider returns an error.
Create test scenarios where the stubRecallFeedbackConfig struct is initialized
with a non-nil err value, and assert that when RecallFeedbackRuntimeConfig
returns an error, the recall feedback system properly disables itself by
ensuring no recall_event is generated and ErrToolDisabled is returned when
attempting to submit feedback. This validates the runtime contract that
configuration provider errors result in safe fail-closed behavior.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 08448ca6-a1f9-4edb-b136-692ac5a7b8c9
📒 Files selected for processing (9)
.github/workflows/snyk.ymlinternal/http/handler/tool_catalog_handler_test.gointernal/http/handler/tool_execute_handler_test.gointernal/mcp/server_test.gointernal/tools/registry/recall_feedback_test.gointernal/tools/registry/runtime_config.gointernal/tools/registry/toolset.gointernal/tools/registry/toolset_test.goweb/tests-compose/compose-portal.spec.ts
💤 Files with no reviewable changes (1)
- .github/workflows/snyk.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tools/registry/runtime_config.go
- internal/mcp/server_test.go
* Address code quality scan follow-ups * Address PR review feedback
There was a problem hiding this comment.
GitVibe Review Matrix
Status: completed
Next state: changes-required
Review synthesis completed for PR #37 at 1d53a91 with 10/10 role results available. Because member outputs disagreed, I inspected the current PR files directly; MCP disabled-tool invocation mapping is fixed, but disabled recall feedback is still exposed through HTTP/MCP discovery because registry.ToolVisible always returns true and tests now enforce that visibility. This contradicts the PR's stated hide-from-discovery requirement, so changes are required before approval.
Next Action
Continue with changes-required.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27730377572
Inline comments: 3
Required Fixes
- internal/tools/registry/runtime_config.go:21-25: ToolVisible always returns true, so disabled recall feedback tools are not hidden from HTTP/MCP discovery. This contradicts the PR requirement to hide recall feedback from live tool discovery when disabled, and the added tests in internal/http/handler/tool_catalog_handler_test.go and internal/http/handler/tool_execute_handler_test.go assert the wrong visible-when-disabled behavior.
gitvibe-for-github
Bot
added
gvi:blocked
There was a problem hiding this comment.
GitVibe Review Matrix
Status: blocked
Next state: blocked
GitVibe paused this run for maintainer review.
Questions
- High-risk prompt-injection content was detected before GitVibe could safely continue.
A. Change the flagged content or safety configuration, or applygit-vibe:accept-riskto accept this prompt-injection input risk for one rerun.
Next Action
Reply in one comment with question numbers and option letters, or write your own answer for any question.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27731291813
Inline comments: 0
Required Fixes
- pull request file README.zh-CN.md: mixes scripts around authority-sensitive terms
- pull request file packages/mcp-proxy/package-lock.json: references a suspicious linked file type
- pull request file scripts/e2e-compose.sh: references a suspicious linked file type
- pull request file web/package-lock.json chunk 1/3: references a suspicious linked file type
- pull request file web/package-lock.json chunk 2/3: references a suspicious linked file type
- pull request file web/package-lock.json chunk 3/3: references a suspicious linked file type
- pull request file web/package-lock.json: references a suspicious linked file type
Accepted Risk
Z-M-Huang accepted this prompt-injection input risk for one rerun.
Accepted at: 2026-06-18T01:50:41.019Z
Accepted workflow run: 27731291813
Accepted workflow attempt: 2
Accepted stages: review-matrix
Artifact title/body SHA: da0125b4190599c943791aabc05b114ec04b48acdfb6cdd9dfdd29b028fe48dd
Pull request head SHA: 7f78c016962142f97ed7d98b8b78d8a0d412eb72
Z-M-Huang
added
the
git-vibe:accept-risk
gitvibe-for-github
Bot
added
gvi:reviewing
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/http/validation/custom.go (1)
66-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on invalid
maxbytestag parameters.Returning
truefor non-numeric params silently disables the size guard. Invalid validator config should fail validation, not bypass it.Proposed fix
import ( "encoding/json" "reflect" + "strconv" "strings" "sync/atomic" "github.com/go-playground/validator/v10" ) @@ - // Parse the max bytes manually - var max int64 - for _, c := range maxBytes { - if c < '0' || c > '9' { - return true // Invalid parameter, skip validation - } - max = max*10 + int64(c-'0') - } + max, err := strconv.ParseInt(maxBytes, 10, 64) + if err != nil || max < 0 { + return false // Invalid parameter, fail closed + } @@ - data, err := json.Marshal(field.Interface()) + data, err := json.Marshal(field.Interface())🤖 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 `@internal/http/validation/custom.go` around lines 66 - 69, The code in the maxBytes validation loop currently returns true when encountering non-numeric characters, which silently disables the size validation guard. Change the return statement from true to false in the loop condition where you check if characters fall outside the '0' to '9' range within the maxBytes iteration. This ensures that invalid maxbytes tag parameters cause validation to fail rather than bypass the size guard.internal/http/control_portal_config_test.go (1)
169-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle invalid boolean input in the recall-feedback test stub.
strconv.ParseBoolerrors are ignored, so invalid values are silently treated asfalse, which can make tests less trustworthy for invalid-input paths.Proposed fix
func (s *controlAppConfigSvc) UpdateRecallFeedbackSettings(_ context.Context, values map[string]string, _, _, _ string) (*domain.RecallFeedbackConfigSettings, error) { if s.updateErr != nil { return nil, s.updateErr } @@ if raw, ok := values[domain.AppConfigRecallFeedbackEnabled]; ok { - enabled, _ := strconv.ParseBool(raw) + enabled, err := strconv.ParseBool(raw) + if err != nil { + return nil, service.ErrInvalidAppConfig + } s.recallRuntime.Enabled = enabled if s.recallSettings != nil { s.recallSettings.Effective.Enabled = 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 `@internal/http/control_portal_config_test.go` around lines 169 - 193, In the UpdateRecallFeedbackSettings method of the controlAppConfigSvc stub, the error returned by strconv.ParseBool when parsing the domain.AppConfigRecallFeedbackEnabled value is being silently ignored. Modify the code to capture and check the error from strconv.ParseBool, and return an appropriate error if the boolean parsing fails instead of allowing invalid values to be silently treated as false.
🧹 Nitpick comments (1)
web/src/user/App.tsx (1)
353-394: Add an error boundary around the lazy-loaded panels to catch chunk load failures.
Suspensewith a fallback only covers loading states; rejected lazy imports still need an error boundary. Without one, a failed chunk load blanks the entire panel instead of showing a recoverable error state.Example implementation
+ <LazyPanelErrorBoundary> <Suspense fallback={<LazyPanelFallback />}> {activeTab === "search" && <SearchPanel api={api} />} {activeTab === "dreams" && <UserDreamsPanel api={api} />} {activeTab === "usage" && <UserTelemetryPanel api={api} />} {activeTab === "facts" && <FactsPanel api={api} />} {activeTab === "claims" && <ClaimsPanel api={api} />} {activeTab === "fragments" && <FragmentsPanel api={api} />} {activeTab === "communities" && <CommunitiesPanel api={api} />} {activeTab === "team" && session?.can_manage_team && ( <TeamManagementPanel api={api} session={session} onTeamUpdated={(team) => setSession((current) => current ? { ...current, team } : current)} /> )} {activeTab === "key" && session && ( <KeyPanel api={api} session={session} onRotated={(rotated) => { if (authMode === "api_key") { sessionStorage.setItem(TOKEN_STORAGE_KEY, rotated.api_key); onTokenChange(rotated.api_key); } setSession((current) => current ? { ...current, key: rotated.key, can_rotate: rotated.key.scopes.includes("write"), can_manage_team: rotated.key.role === "manager", } : current); }} onSSOKeyChanged={(key) => { setSession((current) => current ? { ...current, personal_key: key, can_create_personal_key: false, can_rotate_personal_key: key.scopes.includes("write") && (current.personal_key_max_scopes ?? []).includes("write"), } : current); }} /> )} - </Suspense> + </Suspense> + </LazyPanelErrorBoundary>🤖 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 `@web/src/user/App.tsx` around lines 353 - 394, Wrap the Suspense component containing the lazy-loaded panels (SearchPanel, UserDreamsPanel, UserTelemetryPanel, FactsPanel, ClaimsPanel, FragmentsPanel, CommunitiesPanel, TeamManagementPanel, and KeyPanel) with an error boundary component to catch chunk load failures. The error boundary should provide a recoverable error state instead of allowing failed chunks to blank the entire panel section. Create or use an existing error boundary component that can gracefully handle import failures and display an appropriate error message to the user.
🤖 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 `@internal/service/graphrow/graphrow.go`:
- Around line 65-67: The StringSlice function has inconsistent behavior between
handling []string and []any types. In the case []string branch around line 65,
empty strings are returned as-is, but the case []any branch filters them out.
Modify the case []string branch to also filter out empty strings before
returning, ensuring consistent normalization of empty values regardless of the
input type. This will prevent empty IDs or labels from being propagated when row
values are pre-decoded as []string.
In `@packages/mcp-proxy/package.json`:
- Around line 42-44: The `overrides` field in the package.json for mcp-proxy is
ineffective when the package is installed as a dependency in other projects
because npm only honors overrides in the root package.json. Since `hono` is a
transitive dependency and consumers will have their own root package.json, this
override does not provide downstream protection. Either remove the entire
`overrides` object containing the `hono` version pin, or keep it and add
clarifying documentation in the README or package.json comments explaining that
this override only applies during local development of mcp-proxy itself and does
not affect downstream consumers.
In `@web/src/http.ts`:
- Around line 44-58: The current code silently treats malformed JSON as null for
all responses, which causes downstream errors for successful responses when
callers expect specific payload structures. After the JSON parsing attempt in
the try-catch block, add a check: if response.ok is true and text exists but
payload is null (indicating parsing failed), throw an error to fail fast; keep
the silent null fallback only for error responses where response.ok is false.
This ensures malformed JSON in successful responses is caught immediately rather
than being silently ignored.
---
Outside diff comments:
In `@internal/http/control_portal_config_test.go`:
- Around line 169-193: In the UpdateRecallFeedbackSettings method of the
controlAppConfigSvc stub, the error returned by strconv.ParseBool when parsing
the domain.AppConfigRecallFeedbackEnabled value is being silently ignored.
Modify the code to capture and check the error from strconv.ParseBool, and
return an appropriate error if the boolean parsing fails instead of allowing
invalid values to be silently treated as false.
In `@internal/http/validation/custom.go`:
- Around line 66-69: The code in the maxBytes validation loop currently returns
true when encountering non-numeric characters, which silently disables the size
validation guard. Change the return statement from true to false in the loop
condition where you check if characters fall outside the '0' to '9' range within
the maxBytes iteration. This ensures that invalid maxbytes tag parameters cause
validation to fail rather than bypass the size guard.
---
Nitpick comments:
In `@web/src/user/App.tsx`:
- Around line 353-394: Wrap the Suspense component containing the lazy-loaded
panels (SearchPanel, UserDreamsPanel, UserTelemetryPanel, FactsPanel,
ClaimsPanel, FragmentsPanel, CommunitiesPanel, TeamManagementPanel, and
KeyPanel) with an error boundary component to catch chunk load failures. The
error boundary should provide a recoverable error state instead of allowing
failed chunks to blank the entire panel section. Create or use an existing error
boundary component that can gracefully handle import failures and display an
appropriate error message to the user.
🪄 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: CHILL
Plan: Pro Plus
Run ID: eede1587-1ca9-41de-901a-a65dd6b591d2
⛔ Files ignored due to path filters (2)
packages/mcp-proxy/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
.github/git-vibe.ymlcmd/demo-server/runtime_demo_test.gointernal/config/config.gointernal/domain/audit.gointernal/domain/knowledge_contract.gointernal/embedding/errors.gointernal/embedding/mock_test.gointernal/embedding/provider.gointernal/embedding/sanitize.gointernal/http/control_portal_config_test.gointernal/http/middleware/correlation_id_test.gointernal/http/response/response.gointernal/http/router_protected.gointernal/http/router_public.gointernal/http/validation/custom.gointernal/httperr/errors.gointernal/requestctx/client_ip_test.gointernal/service/app_config_service_test.gointernal/service/claimservice/read.gointernal/service/claimservice/support_test.gointernal/service/classification/lattice_test.gointernal/service/dreamservice/config.gointernal/service/factservice/read.gointernal/service/fragmentidentity/identity.gointernal/service/fragmentidentity/identity_test.gointernal/service/fragmentservice/read.gointernal/service/graphrow/graphrow.gointernal/service/recallservice/recall.gointernal/service/recallservice/recall_fragment_limit_test.gointernal/service/session_cleanup.gointernal/sse/event.gointernal/sse/lifecycle.gointernal/sse/writer.gointernal/storage/neo4j/client_test.gointernal/storage/neo4j/query_guard.gointernal/storage/neo4j/query_guard_test.gointernal/storage/neo4j/writer.gointernal/storage/neo4j/writer_test.gointernal/storage/postgres/client.gointernal/storage/postgres/client_test.gointernal/storage/postgres/embedding_config_repo.gointernal/storage/postgres/embedding_config_repo_test.gointernal/storage/redis/cleanup.gointernal/storage/redis/cleanup_test.gointernal/storage/redis/client_test.gointernal/storage/redis/keys.gopackages/mcp-proxy/package.jsontests/integration/discoverability_harness.gotests/integration/discoverability_harness_test.gotests/uat/phase_boundary_test.goweb/package.jsonweb/src/App.tsxweb/src/api.test.tsweb/src/api.tsweb/src/control/ConfigPanel.tsxweb/src/http.tsweb/src/telemetry/TelemetryDashboard.tsxweb/src/test/setup.tsweb/src/user/App.tsxweb/src/user/api.tsweb/vite.config.tsweb/vite.user.config.ts
💤 Files with no reviewable changes (6)
- internal/storage/neo4j/query_guard.go
- internal/storage/postgres/client_test.go
- internal/storage/neo4j/query_guard_test.go
- internal/storage/neo4j/writer.go
- internal/storage/neo4j/writer_test.go
- internal/service/dreamservice/config.go
✅ Files skipped from review due to trivial changes (31)
- internal/service/classification/lattice_test.go
- internal/http/router_public.go
- internal/storage/postgres/client.go
- web/package.json
- internal/service/claimservice/support_test.go
- internal/embedding/mock_test.go
- internal/storage/postgres/embedding_config_repo_test.go
- internal/embedding/sanitize.go
- internal/storage/redis/cleanup.go
- internal/service/recallservice/recall_fragment_limit_test.go
- internal/embedding/errors.go
- internal/service/fragmentidentity/identity.go
- internal/storage/postgres/embedding_config_repo.go
- internal/service/session_cleanup.go
- internal/sse/lifecycle.go
- internal/storage/redis/cleanup_test.go
- internal/httperr/errors.go
- internal/sse/writer.go
- internal/domain/knowledge_contract.go
- tests/integration/discoverability_harness_test.go
- internal/http/response/response.go
- tests/integration/discoverability_harness.go
- internal/storage/redis/keys.go
- internal/requestctx/client_ip_test.go
- internal/http/middleware/correlation_id_test.go
- internal/storage/redis/client_test.go
- internal/embedding/provider.go
- internal/storage/neo4j/client_test.go
- internal/domain/audit.go
- internal/service/fragmentidentity/identity_test.go
- tests/uat/phase_boundary_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/control/ConfigPanel.tsx
There was a problem hiding this comment.
GitVibe Review Matrix
Status: completed
Next state: changes-required
Review synthesis completed for PR #37 at head 7f78c01 with 9/10 role results available and 1 failed role result. The remaining validated blocker is that runtime tool visibility always returns true, so disabled recall feedback is still exposed through HTTP/MCP discovery despite the PR requirement to hide it when disabled; related tests currently assert the wrong visible-when-disabled behavior.
Next Action
Continue with changes-required.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27731291813
Inline comments: 0
Required Fixes
- internal/tools/registry/runtime_config.go:21-25:
ToolVisiblealways returns true, soSubmitRecallSessionFeedbackToolNameremains visible in HTTP/MCP discovery when recall feedback is disabled. This contradicts the PR requirement to hide recall feedback from live discovery when disabled. - internal/http/handler/tool_catalog_handler_test.go:155-180 and internal/http/handler/tool_execute_handler_test.go:438-456 assert the opposite contract by expecting the disabled recall feedback tool to be present/readable; these tests would preserve the regression instead of catching it.
gitvibe-for-github
Bot
added
gvi:blocked
There was a problem hiding this comment.
GitVibe Review Matrix
Status: blocked
Next state: blocked
GitVibe paused this run for maintainer review.
Questions
- High-risk prompt-injection content was detected before GitVibe could safely continue.
A. Change the flagged content or safety configuration, or applygit-vibe:accept-riskto accept this prompt-injection input risk for one rerun.
Next Action
Reply in one comment with question numbers and option letters, or write your own answer for any question.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27757222833
Inline comments: 0
Required Fixes
- comment 4724898153 chunk 1/2: mixes scripts around authority-sensitive terms
- pull request file README.zh-CN.md: mixes scripts around authority-sensitive terms
- pull request file packages/mcp-proxy/package-lock.json: references a suspicious linked file type
- pull request file scripts/e2e-compose.sh: references a suspicious linked file type
- pull request file web/package-lock.json chunk 1/3: references a suspicious linked file type
- pull request file web/package-lock.json chunk 2/3: references a suspicious linked file type
- pull request file web/package-lock.json chunk 3/3: references a suspicious linked file type
- pull request file web/package-lock.json: references a suspicious linked file type
Accepted Risk
Z-M-Huang accepted this prompt-injection input risk for one rerun.
Accepted at: 2026-06-18T11:49:03.635Z
Accepted workflow run: 27757222833
Accepted workflow attempt: 2
Accepted stages: review-matrix
Artifact title/body SHA: da0125b4190599c943791aabc05b114ec04b48acdfb6cdd9dfdd29b028fe48dd
Pull request head SHA: f2e356851a3e6bacff73fa298a8452fbcc04e929
Z-M-Huang
added
the
git-vibe:accept-risk
gitvibe-for-github
Bot
added
gvi:reviewing
There was a problem hiding this comment.
GitVibe Review Matrix
Status: completed
Next state: review-passed
Review synthesis completed for PR #37 at 40f4fdb with 9/10 role results available and 1 member result missing. Direct inspection of the disputed current-head files shows the earlier disabled recall-feedback discovery findings are resolved, and no evidence-backed required fixes remain.
Next Action
Continue with review-passed.
Result
When present, required fixes are posted as pull request review comments. Full structured output remains in the workflow run summary.
Workflow run: https://github.com/markhuangai/dense-mem/actions/runs/27757878313
gitvibe-for-github
Bot
added
gvi:ready-for-approval
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
2 participants
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
Testing
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation