feat: memory enhancements phase 1 - #63
Merged
Merged
Conversation
added 5 commits
July 8, 2026 00:01
|
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds privacy redaction, deduplication, token-budget truncation, and circuit breaker utilities, wires them into memory tools, and updates the roadmap with a migration section. ChangesMemory Tools Reliability and Utilities
Estimated code review effort: 4 (Complex) | ~45 minutes Roadmap Documentation
Sequence Diagram(s)sequenceDiagram
participant Client
participant memory_remember
participant strip_secrets
participant _DedupCache
Client->>memory_remember: call(key, value, session_id)
memory_remember->>strip_secrets: strip_secrets(value)
strip_secrets-->>memory_remember: sanitized value
memory_remember->>_DedupCache: check hash within TTL
_DedupCache-->>memory_remember: duplicate or new
memory_remember-->>Client: RememberResult(status)
Related issues: None specified. Related PRs: None specified. Suggested labels: enhancement, reliability, security Suggested reviewers: None specified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryPhase 1 memory enhancements adding SHA-256 deduplication, token-budget truncation, a privacy filter for secrets, and a circuit breaker utility. The dedup and privacy filter are correctly wired into
Confidence Score: 3/5Not safe to merge without addressing the token-budget truncation bug and the unconnected circuit breaker. The token-budget truncation function ( mcp_server/tools_layer.py ( Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant memory_remember
participant strip_secrets
participant _DedupCache
participant CoreMemory
Client->>memory_remember: key, value, session_id
memory_remember->>strip_secrets: strip_secrets(value)
strip_secrets-->>memory_remember: sanitized_value
alt session_id provided
memory_remember->>_DedupCache: is_duplicate(session_id, key, sanitized_value)
alt duplicate within TTL
_DedupCache-->>memory_remember: True
memory_remember-->>Client: "status=skipped, reason=duplicate_within_ttl"
else not duplicate
_DedupCache-->>memory_remember: False (records hash+timestamp)
memory_remember->>CoreMemory: remember(key, sanitized_value, importance)
CoreMemory-->>memory_remember: entry_id
memory_remember-->>Client: "status=ok, entry_id"
end
else no session_id
memory_remember->>CoreMemory: remember(key, sanitized_value, importance)
CoreMemory-->>memory_remember: entry_id
memory_remember-->>Client: "status=ok, entry_id"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant memory_remember
participant strip_secrets
participant _DedupCache
participant CoreMemory
Client->>memory_remember: key, value, session_id
memory_remember->>strip_secrets: strip_secrets(value)
strip_secrets-->>memory_remember: sanitized_value
alt session_id provided
memory_remember->>_DedupCache: is_duplicate(session_id, key, sanitized_value)
alt duplicate within TTL
_DedupCache-->>memory_remember: True
memory_remember-->>Client: "status=skipped, reason=duplicate_within_ttl"
else not duplicate
_DedupCache-->>memory_remember: False (records hash+timestamp)
memory_remember->>CoreMemory: remember(key, sanitized_value, importance)
CoreMemory-->>memory_remember: entry_id
memory_remember-->>Client: "status=ok, entry_id"
end
else no session_id
memory_remember->>CoreMemory: remember(key, sanitized_value, importance)
CoreMemory-->>memory_remember: entry_id
memory_remember-->>Client: "status=ok, entry_id"
end
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
mcp_server/utils/circuit_breaker.py:38-50
**Circuit breaker defined but never wired to any call path**
`CircuitBreaker` / `breaker_registry` are exported from `utils/__init__.py` but are not imported or used anywhere in `tools_layer.py` or any other production module in the diff. Every LLM/embedding call path remains unprotected. The PR description states this feature "prevents cascading LLM/embedding failures," but that protection is not active — the class exists purely as dead infrastructure.
Reviews (3): Last reviewed commit: "fix: format code with ruff" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+103
to
+112
| def allow_request(self) -> bool: | ||
| self._total_requests += 1 | ||
| current_state = self.state | ||
|
|
||
| if current_state == CircuitState.CLOSED: | ||
| return True | ||
| if current_state == CircuitState.HALF_OPEN: | ||
| return True | ||
| self._total_rejections += 1 | ||
| return False |
There was a problem hiding this comment.
allow_request() double-counts _total_requests when paired with the context manager
allow_request() increments _total_requests at line 104, then record_success() / record_failure() (called from __exit__) each increment it again. Any caller using with breaker as allowed: will count every request twice, making total_requests in get_metrics() unreliable as a failure-rate denominator.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/utils/circuit_breaker.py
Line: 103-112
Comment:
**`allow_request()` double-counts `_total_requests` when paired with the context manager**
`allow_request()` increments `_total_requests` at line 104, then `record_success()` / `record_failure()` (called from `__exit__`) each increment it again. Any caller using `with breaker as allowed:` will count every request twice, making `total_requests` in `get_metrics()` unreliable as a failure-rate denominator.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
215
to
228
| key: Fact key (e.g. "name", "language", "principle") | ||
| value: Fact value | ||
| importance: Importance score 0.0-1.0 (default 0.5) | ||
| session_id: Session ID for dedup (optional) | ||
| """ | ||
| # SHA-256 dedup: skip identical calls within 5min window | ||
| value = strip_secrets(value) | ||
| if session_id and _dedup_cache.is_duplicate(session_id, key, value): | ||
| logger.info("Dedup: skipping identical remember key=%s user=%s", key, user_id) | ||
| return RememberResult(status="skipped", reason="duplicate_within_ttl").dict() | ||
|
|
||
| app = _get_ctx(ctx) | ||
| layer = _validate_layer(layer) | ||
| metrics.inc("tool_calls") |
There was a problem hiding this comment.
memory_remember.value
strip_secrets is applied to value here, but other write tools (e.g., add_observation, record_episode) receive free-form text that is never sanitized. A credential in an observation or episode body would be stored in plain text. The key parameter of memory_remember is also not filtered.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/tools_layer.py
Line: 215-228
Comment:
**Privacy filter only covers `memory_remember.value`**
`strip_secrets` is applied to `value` here, but other write tools (e.g., `add_observation`, `record_episode`) receive free-form text that is never sanitized. A credential in an observation or episode body would be stored in plain text. The `key` parameter of `memory_remember` is also not filtered.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@mcp_server/tools_layer.py`:
- Around line 73-98: The truncation logic in _truncate_to_budget does not
actually guarantee the returned text stays within max_tokens because it only
uses a char-based cutoff and then appends the suffix afterward. Update
_truncate_to_budget (and, if needed, _estimate_tokens) so the suffix is counted
against the budget and the final returned string always fits within max_tokens
for both ASCII and CJK-heavy input. Keep the fix localized to the token
estimation/truncation helpers in tools_layer.py.
- Around line 55-58: The deduplication key in is_duplicate currently hashes only
a truncated input_text prefix, so distinct observations with the same first 500
characters can be skipped as duplicates. Update the hashing in is_duplicate to
use the full sanitized input_text value rather than slicing it, while keeping
the session_id and tool components unchanged so the duplicate check still scopes
correctly.
In `@mcp_server/utils/__init__.py`:
- Line 3: The __all__ declaration in mcp_server.utils is exporting the class
object instead of its name string, which can trip type checking and wildcard
imports. Update the __all__ list in the package initializer so it contains the
string name for CircuitBreaker, and ensure any similar export entries in this
module follow the same string-based pattern.
In `@mcp_server/utils/circuit_breaker.py`:
- Around line 84-91: The circuit breaker is double-counting accepted requests
because allow_request() already increments _total_requests, and
record_success()/record_failure() increment it again. Update
CircuitBreaker.record_success and CircuitBreaker.record_failure to stop
incrementing _total_requests, and keep the request count update only in
allow_request so normal and context-manager paths share the same accounting.
Make the change consistently across the affected CircuitBreaker methods
referenced in the diff so the metrics stay accurate.
- Around line 41-47: Add explicit return annotations to the public APIs in
CircuitBreaker and its registry helpers: update the CircuitBreaker.__init__,
record_success, record_failure, reset, __enter__, and __exit__ signatures, plus
the registry reset method, so every public function/method in this module has a
return type. Keep the existing behavior unchanged while adding the appropriate
annotations to the named methods so they match the “Type annotations on public
functions” requirement.
- Around line 11-27: Fix the Ruff lint failures in circuit_breaker.py by
removing the unused typing.Any import (or referencing it where it is actually
needed) and cleaning up any blank lines that contain trailing whitespace
throughout the CircuitBreaker module. Check the import block and the surrounding
helper methods/functions in this file for the unused symbol and whitespace-only
lines so the entire diff passes linting.
- Around line 109-110: The CircuitBreaker.allow_request behavior for
CircuitState.HALF_OPEN currently permits every caller through, which violates
the single-probe recovery intent. Update the CircuitBreaker logic so only one
in-flight trial request is allowed while half-open, using the existing
_half_open_probe_in_flight guard inside allow_request, and make sure
record_success, record_failure, and reset clear that flag so the breaker can
recover cleanly after the probe completes.
In `@mcp_server/utils/privacy.py`:
- Around line 5-23: The redaction patterns in _CREDENTIAL_PATTERNS are too
narrow, so strip_secrets() can still let common secrets through before
memory_remember persists them. Expand the matching in
mcp_server/utils/privacy.py to cover key/value-style secrets like api_key,
password, client_secret, and case-insensitive private/secret/credentials tags,
and ensure strip_secrets() uses those updated patterns everywhere values are
stored.
🪄 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: 43160a64-483f-44c0-b4e2-769170d43827
📒 Files selected for processing (5)
ROADMAP.mdmcp_server/tools_layer.pymcp_server/utils/__init__.pymcp_server/utils/circuit_breaker.pymcp_server/utils/privacy.py
Comment on lines
+73
to
+98
| def _estimate_tokens(text: str) -> int: | ||
| if not text: | ||
| return 0 | ||
| cjk_count = len(re.findall(r'[\u4e00-\u9fff\u3040-\u309f\u30a0-\u30ff]', text)) | ||
| remaining_chars = len(text) - cjk_count | ||
| non_cjk_tokens = remaining_chars // CHARS_PER_TOKEN | ||
| return cjk_count + non_cjk_tokens | ||
|
|
||
|
|
||
| def _truncate_to_budget(text: str, max_tokens: int) -> tuple: | ||
| estimated = _estimate_tokens(text) | ||
| if estimated <= max_tokens: | ||
| return text, False | ||
| char_limit = max_tokens * CHARS_PER_TOKEN | ||
| lines = text.split('\\n') | ||
| result_lines = [] | ||
| current_len = 0 | ||
| for line in lines: | ||
| line_len = len(line) + 1 | ||
| if current_len + line_len > char_limit: | ||
| break | ||
| result_lines.append(line) | ||
| current_len += line_len | ||
| truncated = '\\n'.join(result_lines) | ||
| truncated += '\\n[...truncated to token budget]' | ||
| return truncated, True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make truncation enforce the token budget.
The current path converts 2000 tokens to 8000 chars, then appends a suffix. CJK-heavy context can still exceed 2000 estimated tokens, and exact-fit ASCII can exceed after the suffix is added.
🐛 Suggested budget-aware truncation
def _estimate_tokens(text: str) -> int:
@@
- non_cjk_tokens = remaining_chars // CHARS_PER_TOKEN
+ non_cjk_tokens = (remaining_chars + CHARS_PER_TOKEN - 1) // CHARS_PER_TOKEN
return cjk_count + non_cjk_tokens
def _truncate_to_budget(text: str, max_tokens: int) -> tuple:
estimated = _estimate_tokens(text)
if estimated <= max_tokens:
return text, False
- char_limit = max_tokens * CHARS_PER_TOKEN
- lines = text.split('\\n')
- result_lines = []
- current_len = 0
- for line in lines:
- line_len = len(line) + 1
- if current_len + line_len > char_limit:
- break
- result_lines.append(line)
- current_len += line_len
- truncated = '\\n'.join(result_lines)
- truncated += '\\n[...truncated to token budget]'
+ suffix = "\n[...truncated to token budget]"
+ content_budget = max(0, max_tokens - _estimate_tokens(suffix))
+ lo, hi = 0, len(text)
+ while lo < hi:
+ mid = (lo + hi + 1) // 2
+ if _estimate_tokens(text[:mid]) <= content_budget:
+ lo = mid
+ else:
+ hi = mid - 1
+ truncated = text[:lo].rstrip() + suffix
return truncated, True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _estimate_tokens(text: str) -> int: | |
| if not text: | |
| return 0 | |
| cjk_count = len(re.findall(r'[\u4e00-\u9fff\u3040-\u309f\u30a0-\u30ff]', text)) | |
| remaining_chars = len(text) - cjk_count | |
| non_cjk_tokens = remaining_chars // CHARS_PER_TOKEN | |
| return cjk_count + non_cjk_tokens | |
| def _truncate_to_budget(text: str, max_tokens: int) -> tuple: | |
| estimated = _estimate_tokens(text) | |
| if estimated <= max_tokens: | |
| return text, False | |
| char_limit = max_tokens * CHARS_PER_TOKEN | |
| lines = text.split('\\n') | |
| result_lines = [] | |
| current_len = 0 | |
| for line in lines: | |
| line_len = len(line) + 1 | |
| if current_len + line_len > char_limit: | |
| break | |
| result_lines.append(line) | |
| current_len += line_len | |
| truncated = '\\n'.join(result_lines) | |
| truncated += '\\n[...truncated to token budget]' | |
| return truncated, True | |
| def _estimate_tokens(text: str) -> int: | |
| if not text: | |
| return 0 | |
| cjk_count = len(re.findall(r'[\u4e00-\u9fff\u3040-\u309f\u30a0-\u30ff]', text)) | |
| remaining_chars = len(text) - cjk_count | |
| non_cjk_tokens = (remaining_chars + CHARS_PER_TOKEN - 1) // CHARS_PER_TOKEN | |
| return cjk_count + non_cjk_tokens | |
| def _truncate_to_budget(text: str, max_tokens: int) -> tuple: | |
| estimated = _estimate_tokens(text) | |
| if estimated <= max_tokens: | |
| return text, False | |
| suffix = "\n[...truncated to token budget]" | |
| content_budget = max(0, max_tokens - _estimate_tokens(suffix)) | |
| lo, hi = 0, len(text) | |
| while lo < hi: | |
| mid = (lo + hi + 1) // 2 | |
| if _estimate_tokens(text[:mid]) <= content_budget: | |
| lo = mid | |
| else: | |
| hi = mid - 1 | |
| truncated = text[:lo].rstrip() + suffix | |
| return truncated, True |
🧰 Tools
🪛 GitHub Actions: CI / 0_lint.txt
[error] 76-76: F821 Undefined name re (used in re.findall(...)).
🪛 GitHub Actions: CI / 7_typecheck.txt
[error] 76-76: mypy: Name "re" is not defined [name-defined]
🪛 GitHub Actions: CI / lint
[error] 76-76: ruff F821: Undefined name re.
🪛 GitHub Actions: CI / typecheck
[error] 76-76: mypy: Name "re" is not defined [name-defined]
🤖 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 `@mcp_server/tools_layer.py` around lines 73 - 98, The truncation logic in
_truncate_to_budget does not actually guarantee the returned text stays within
max_tokens because it only uses a char-based cutoff and then appends the suffix
afterward. Update _truncate_to_budget (and, if needed, _estimate_tokens) so the
suffix is counted against the budget and the final returned string always fits
within max_tokens for both ASCII and CJK-heavy input. Keep the fix localized to
the token estimation/truncation helpers in tools_layer.py.
Comment on lines
+41
to
+47
| def __init__( | ||
| self, | ||
| threshold: int = 3, | ||
| recovery_timeout: float = 30.0, | ||
| name: str = "default", | ||
| on_state_change: Optional[Callable] = None, | ||
| ): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add return annotations to public methods.
Several public methods omit return annotations (__init__, record_success, record_failure, reset, context-manager methods, registry reset). As per path instructions, mcp_server/**/*.py: “Type annotations on public functions”.
Also applies to: 84-142, 145-167
🤖 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 `@mcp_server/utils/circuit_breaker.py` around lines 41 - 47, Add explicit
return annotations to the public APIs in CircuitBreaker and its registry
helpers: update the CircuitBreaker.__init__, record_success, record_failure,
reset, __enter__, and __exit__ signatures, plus the registry reset method, so
every public function/method in this module has a return type. Keep the existing
behavior unchanged while adding the appropriate annotations to the named methods
so they match the “Type annotations on public functions” requirement.
Source: Path instructions
Comment on lines
+109
to
+110
| if current_state == CircuitState.HALF_OPEN: | ||
| return True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Limit half-open to a single recovery probe.
The docstring says half-open allows one request, but this returns True for every caller until a result is recorded, allowing a burst against a recovering LLM/embedding dependency.
🛡️ Suggested probe guard
self._state_changes = 0
+ self._half_open_probe_in_flight = False
@@
if current_state == CircuitState.HALF_OPEN:
+ if self._half_open_probe_in_flight:
+ self._total_rejections += 1
+ return False
+ self._half_open_probe_in_flight = True
return TrueAlso clear _half_open_probe_in_flight in record_success, record_failure, and reset.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if current_state == CircuitState.HALF_OPEN: | |
| return True | |
| if current_state == CircuitState.HALF_OPEN: | |
| if self._half_open_probe_in_flight: | |
| self._total_rejections += 1 | |
| return False | |
| self._half_open_probe_in_flight = True | |
| return True |
🤖 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 `@mcp_server/utils/circuit_breaker.py` around lines 109 - 110, The
CircuitBreaker.allow_request behavior for CircuitState.HALF_OPEN currently
permits every caller through, which violates the single-probe recovery intent.
Update the CircuitBreaker logic so only one in-flight trial request is allowed
while half-open, using the existing _half_open_probe_in_flight guard inside
allow_request, and make sure record_success, record_failure, and reset clear
that flag so the breaker can recover cleanly after the probe completes.
Comment on lines
+5
to
+23
| _CREDENTIAL_PATTERNS: List[Pattern] = [ | ||
| re.compile(r"\b(sk-[A-Za-z0-9]{20,})\b"), | ||
| re.compile(r"\b(sk-ant-[A-Za-z0-9-]{20,})\b"), | ||
| re.compile(r"\b(ghp_[A-Za-z0-9]{36})\b"), | ||
| re.compile(r"\b(gho_[A-Za-z0-9]{36})\b"), | ||
| re.compile(r"\b(ghs_[A-Za-z0-9]{36})\b"), | ||
| re.compile(r"\b(ghr_[A-Za-z0-9]{36})\b"), | ||
| re.compile(r"\b(xox[baprs]-[A-Za-z0-9-]{20,})\b"), | ||
| re.compile(r"\b(AKIA[0-9A-Z]{16})\b"), | ||
| re.compile(r"\b(AIza[0-9A-Za-z_-]{35})\b"), | ||
| re.compile(r"\b(sk_live_[0-9a-zA-Z]{24,})\b"), | ||
| re.compile(r"\b(pk_live_[0-9a-zA-Z]{24,})\b"), | ||
| re.compile(r"\b(sk_test_[0-9a-zA-Z]{24,})\b"), | ||
| re.compile(r"\b([0-9]{10}:[A-Za-z0-9_-]{35})\b"), | ||
| re.compile(r"\b(Bearer\s+[A-Za-z0-9_\-\.]{20,})\b", re.IGNORECASE), | ||
| re.compile(r"<private>.*?</private>", re.DOTALL), | ||
| re.compile(r"<secret>.*?</secret>", re.DOTALL), | ||
| re.compile(r"<credentials>.*?</credentials>", re.DOTALL), | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Broaden redaction before storing memories.
memory_remember relies on strip_secrets(value) before persistence, but these patterns miss common secret forms like api_key=..., password: ..., client_secret=..., and case-varied private tags. Those values can still be stored in memory.
🛡️ Suggested redaction expansion
_CREDENTIAL_PATTERNS: List[Pattern] = [
+ re.compile(
+ r"\b(api[_-]?key|access[_-]?token|auth[_-]?token|client[_-]?secret|secret|password|passwd|pwd)\b"
+ r"\s*[:=]\s*['\"]?[^'\"\s,;]+['\"]?",
+ re.IGNORECASE,
+ ),
re.compile(r"\b(sk-[A-Za-z0-9]{20,})\b"),
@@
- re.compile(r"<private>.*?</private>", re.DOTALL),
- re.compile(r"<secret>.*?</secret>", re.DOTALL),
- re.compile(r"<credentials>.*?</credentials>", re.DOTALL),
+ re.compile(r"<private>.*?</private>", re.DOTALL | re.IGNORECASE),
+ re.compile(r"<secret>.*?</secret>", re.DOTALL | re.IGNORECASE),
+ re.compile(r"<credentials>.*?</credentials>", re.DOTALL | re.IGNORECASE),
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _CREDENTIAL_PATTERNS: List[Pattern] = [ | |
| re.compile(r"\b(sk-[A-Za-z0-9]{20,})\b"), | |
| re.compile(r"\b(sk-ant-[A-Za-z0-9-]{20,})\b"), | |
| re.compile(r"\b(ghp_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(gho_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(ghs_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(ghr_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(xox[baprs]-[A-Za-z0-9-]{20,})\b"), | |
| re.compile(r"\b(AKIA[0-9A-Z]{16})\b"), | |
| re.compile(r"\b(AIza[0-9A-Za-z_-]{35})\b"), | |
| re.compile(r"\b(sk_live_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b(pk_live_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b(sk_test_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b([0-9]{10}:[A-Za-z0-9_-]{35})\b"), | |
| re.compile(r"\b(Bearer\s+[A-Za-z0-9_\-\.]{20,})\b", re.IGNORECASE), | |
| re.compile(r"<private>.*?</private>", re.DOTALL), | |
| re.compile(r"<secret>.*?</secret>", re.DOTALL), | |
| re.compile(r"<credentials>.*?</credentials>", re.DOTALL), | |
| ] | |
| _CREDENTIAL_PATTERNS: List[Pattern] = [ | |
| re.compile( | |
| r"\b(api[_-]?key|access[_-]?token|auth[_-]?token|client[_-]?secret|secret|password|passwd|pwd)\b" | |
| r"\s*[:=]\s*['\"]?[^'\"\s,;]+['\"]?", | |
| re.IGNORECASE, | |
| ), | |
| re.compile(r"\b(sk-[A-Za-z0-9]{20,})\b"), | |
| re.compile(r"\b(sk-ant-[A-Za-z0-9-]{20,})\b"), | |
| re.compile(r"\b(ghp_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(gho_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(ghs_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(ghr_[A-Za-z0-9]{36})\b"), | |
| re.compile(r"\b(xox[baprs]-[A-Za-z0-9-]{20,})\b"), | |
| re.compile(r"\b(AKIA[0-9A-Z]{16})\b"), | |
| re.compile(r"\b(AIza[0-9A-Za-z_-]{35})\b"), | |
| re.compile(r"\b(sk_live_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b(pk_live_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b(sk_test_[0-9a-zA-Z]{24,})\b"), | |
| re.compile(r"\b([0-9]{10}:[A-Za-z0-9_-]{35})\b"), | |
| re.compile(r"\b(Bearer\s+[A-Za-z0-9_\-\.]{20,})\b", re.IGNORECASE), | |
| re.compile(r"<private>.*?</private>", re.DOTALL | re.IGNORECASE), | |
| re.compile(r"<secret>.*?</secret>", re.DOTALL | re.IGNORECASE), | |
| re.compile(r"<credentials>.*?</credentials>", re.DOTALL | re.IGNORECASE), | |
| ] |
🤖 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 `@mcp_server/utils/privacy.py` around lines 5 - 23, The redaction patterns in
_CREDENTIAL_PATTERNS are too narrow, so strip_secrets() can still let common
secrets through before memory_remember persists them. Expand the matching in
mcp_server/utils/privacy.py to cover key/value-style secrets like api_key,
password, client_secret, and case-insensitive private/secret/credentials tags,
and ensure strip_secrets() uses those updated patterns everywhere values are
stored.
Comment on lines
+38
to
+50
| class CircuitBreaker: | ||
| """Circuit breaker with configurable threshold and recovery timeout.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| threshold: int = 3, | ||
| recovery_timeout: float = 30.0, | ||
| name: str = "default", | ||
| on_state_change: Optional[Callable] = None, | ||
| ): | ||
| self.threshold = threshold | ||
| self.recovery_timeout = recovery_timeout | ||
| self.name = name |
There was a problem hiding this comment.
CircuitBreaker / breaker_registry are exported from utils/__init__.py but are not imported or used anywhere in tools_layer.py or any other production module in the diff. Every LLM/embedding call path remains unprotected. The PR description states this feature "prevents cascading LLM/embedding failures," but that protection is not active — the class exists purely as dead infrastructure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/utils/circuit_breaker.py
Line: 38-50
Comment:
**Circuit breaker defined but never wired to any call path**
`CircuitBreaker` / `breaker_registry` are exported from `utils/__init__.py` but are not imported or used anywhere in `tools_layer.py` or any other production module in the diff. Every LLM/embedding call path remains unprotected. The PR description states this feature "prevents cascading LLM/embedding failures," but that protection is not active — the class exists purely as dead infrastructure.
How can I resolve this? If you propose a fix, please make it concise.
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
Phase 1 of memory system enhancements. Adds security, reliability, and performance features.
Changes
Testing
Files Changed
Summary by CodeRabbit