Feat/natural language goal creation - #1007
Closed
cyb3ralee wants to merge 64 commits into
Closed
Conversation
Bump ethnum 1.5.0 -> 1.5.3 in packages/contracts so the contract crate compiles on the current stable toolchain (rustc 1.97.1). ethnum 1.5.0 fails with error[E0512] transmuting () into TryFromIntError, which no longer share a size; the crate fixed it in 1.5.1. Bump apps/api to Go 1.25.12 and golang.org/x/text v0.39.0 to clear the two govulncheck findings not in the allowlist: GO-2026-5856 (crypto/tls, fixed in go1.25.12) and GO-2026-5970 (x/text, fixed in v0.39.0).
… for chat/analyze (Suncrest-Labs#875) * feat(intelligence): add prompt-injection and output-safety guardrails Claude calls in the chat and analyze paths had no defense against prompt injection or system-prompt extraction, and recommendation output wasn't schema-enforced. Add input screening (regex-based, logs request_id + a non-reversible fingerprint, never raw content), a hardened system prompt with an explicit trust boundary and tagged untrusted-content wrapping, deterministic history/message bounding, and output post-processing that strips leaked system-prompt text and enforces a non-model-controlled disclaimer on /analyze and related endpoints. * fix(intelligence): close remaining guardrail gaps from review - Validate inbound X-Request-Id against a bounded safe charset before trusting it in state/headers/logs, falling back to a fresh UUID otherwise (prevents log/header injection via a client-supplied header). - Fix the chat streaming leak-redaction buffer to retain a sanitized lookback tail on flush instead of resetting to empty, so a system-prompt marker split across two deltas is still caught. - Wrap the remaining unwrapped context data interpolated into the recommend/vault and analyze prompts (positions, vault/user context lines) in the same trust-boundary tags used elsewhere. - Sanitize the few model-derived output fields that were missed: confidence_reason/data_freshness in Recommendation, insight card action.label/href, and deposit schedule note.
…ured output - Add goal_extractor service with Claude structured output - Add natural_language_goal router for API endpoints - Extract name, amount, deadline, category from natural language - Validate deterministically after extraction - Surface ambiguities for confirmation, never guess - Prompt injection resistant - Add comprehensive tests - All ruff checks passing Closes Suncrest-Labs#853
✅ Deploy Preview for nesterhq canceled.
|
✅ Deploy Preview for nesterdapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedToo many files! This PR contains 526 files, which is 226 over the limit of 300. To get a review, narrow the scope: Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (532)
You can disable this status message by setting the WalkthroughAdds intelligence-service guardrails, request correlation, Claude prompt/output handling, and authenticated natural-language savings-goal extraction with deterministic validation and confirmation endpoints. It also adds related tests and updates Go toolchain dependencies. ChangesIntelligence service
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
Closed
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
apps/intelligence/app/services/prometheus.py (1)
418-446: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBuffer isn't updated to the sanitized value when
emit_len <= 0.When
strip_system_prompt_leakageshrinkspendingenough thatemit_len <= 0,pendingkeeps its original unsanitized content instead of being set tosanitized. Nothing unsafe reaches the client (everyyieldusessanitized), and the end-of-stream flush re-sanitizes anyway, so this is a very narrow, low-impact edge case rather than a real leak.♻️ Optional tightening
sanitized = guardrails.strip_system_prompt_leakage( pending, request_id=request_id ) emit_len = len(sanitized) - _LEAK_OVERLAP if emit_len > 0: safe_chunk = sanitized[:emit_len].replace("\n", "\\n") yield f"data: {safe_chunk}\n\n" pending = sanitized[emit_len:] + else: + pending = sanitized🤖 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 `@apps/intelligence/app/services/prometheus.py` around lines 418 - 446, Update the streaming buffer logic around strip_system_prompt_leakage so pending is replaced with the sanitized value even when emit_len is non-positive. Preserve the existing yield behavior for positive emit_len, while ensuring subsequent iterations never retain the unsanitized pending content.apps/intelligence/app/routers/analyze.py (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated request-id fallback logic across three routers.
analyze.py,chat.py, andcoaching.pyeach independently derive a request-correlation id with the samegetattr(request.state, "request_id", "") or str(uuid.uuid4())expression; a shared helper (e.g. in a common router-utils module) would keep this single-sourced now thatadd_request_idmiddleware inmain.pyis the actual source of truth.
apps/intelligence/app/routers/analyze.py#L52-L55: promote_request_idinto a shared utility importable by the other routers.apps/intelligence/app/routers/chat.py#L48-L48: replace the inline expression with the shared helper.apps/intelligence/app/routers/coaching.py#L27-L27: replace the inline expression with the shared helper.🤖 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 `@apps/intelligence/app/routers/analyze.py` around lines 52 - 55, Promote analyze.py’s _request_id into a shared router utility, preserving its existing middleware-backed fallback behavior, and import it wherever request correlation IDs are needed. Update apps/intelligence/app/routers/analyze.py lines 52-55 to define or import the shared helper, replace the inline expression at apps/intelligence/app/routers/chat.py line 48 with that helper, and replace the inline expression at apps/intelligence/app/routers/coaching.py line 27 likewise.apps/intelligence/app/services/claude.py (2)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo module docstrings — the second is a dead string expression.
Only line 1 becomes
__doc__; lines 3-11 are an unused literal. Merge them.♻️ Merge docstrings
-"""Claude client configuration.""" - -"""Claude client and per-user tone/style prompt helpers (`#927`). +"""Claude client configuration and per-user tone/style prompt helpers (`#927`).🤖 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 `@apps/intelligence/app/services/claude.py` around lines 1 - 11, Merge the standalone “Claude client configuration” docstring with the following module documentation into one module-level docstring, preserving the full description and ensuring it is the first statement in the module so the combined text becomes __doc__.
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the model from settings at call time.
MODEL_IDfreezessettings.anthropic_modelat import, so any runtime/reload of settings is ignored, and the indirection also hides the pin from the repo's model-pin regression test (see the related comment ingoal_extractor.py).♻️ Resolve model per call
-# Use the configured model from settings -MODEL_ID = settings.anthropic_model - - def get_client(): return client -def get_model_id(): - return MODEL_ID +def get_model_id() -> str: + return settings.anthropic_model🤖 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 `@apps/intelligence/app/services/claude.py` around lines 20 - 29, Update get_model_id() to read and return settings.anthropic_model at call time instead of returning the import-time MODEL_ID constant; remove the frozen MODEL_ID indirection so runtime settings reloads and model-pin checks observe the configured value.apps/intelligence/app/services/guardrails.py (1)
177-182: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTag stripping in
_wrapis exact-literal only.
replace(close_tag, "")misses whitespace/case variants (</ user_message>,</USER_MESSAGE>).screen_input'sfake_boundary_tagpattern covers the live path, so this is defense-in-depth for callers that wrap without screening (e.g.wrap_context_block).♻️ Regex-based tag neutralisation
-def _wrap(tag: str, content: str, max_chars: int) -> str: - truncated = content[:max_chars] - close_tag = f"</{tag}>" - # Neutralise any attempt to smuggle a fake closing tag to escape the block. - escaped = truncated.replace(close_tag, "").replace(f"<{tag}>", "") - return f"<{tag}>\n{escaped}\n{close_tag}" +def _wrap(tag: str, content: str, max_chars: int) -> str: + truncated = content[:max_chars] + # Neutralise any attempt to smuggle a fake boundary tag (including + # whitespace/case variants) to escape the block. + escaped = re.sub( + rf"</?\s*{re.escape(tag)}\s*>", "", truncated, flags=re.IGNORECASE + ) + return f"<{tag}>\n{escaped}\n</{tag}>"🤖 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 `@apps/intelligence/app/services/guardrails.py` around lines 177 - 182, Update `_wrap` to neutralize fake opening and closing tags using case-insensitive regex matching that permits whitespace inside the tag syntax, rather than exact-literal replacement. Preserve truncation, the surrounding wrapper tags, and existing behavior for valid content; apply the same neutralization to both opening and closing forms.
🤖 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 `@apps/intelligence/app/routers/natural_language_goal.py`:
- Around line 100-105: The natural-language goal flow currently fabricates
success in the TODO return block without creating a goal. Replace this response
in the surrounding goal-validation handler with a call to the existing
relay/backend validated creation service, pass the validated goal data, and
return the service’s actual creation result and goal identifier.
- Around line 111-113: Update the exception handling around goal creation so
HTTPException instances, including the intentional 400 validation error, are
re-raised unchanged before the generic Exception handler. Keep logging and
converting unexpected exceptions to the existing 500 response in the
natural-language goal creation flow.
In `@apps/intelligence/app/services/goal_extractor.py`:
- Around line 83-111: Replace the bespoke `_check_injection` pattern scan with
the existing `guardrails.screen_input` behavior, avoiding duplicate broader
filtering and preserving legitimate goal descriptions. In
`_build_extraction_prompt`, wrap the user message with
`guardrails.wrap_user_content` before interpolation so it is explicitly
separated from the extraction instructions.
- Around line 104-111: Update _build_extraction_prompt to derive the displayed
current date using the user's configured user_timezone rather than naive
server-local datetime.now(). Preserve the existing YYYY-MM-DD formatting so
relative-date extraction and subsequent validation use the same user-local
calendar date.
- Around line 64-70: Update the messages.create call in the goal extraction flow
to pass settings.anthropic_model directly as the model value, or rename and use
the instance attribute to clearly include anthropic_model. Keep the pinned model
visible at this call site so the guardrail regression test recognizes it.
- Around line 126-153: Update the deadline parsing in the goal extraction
validation block to localize naive datetimes with the pytz timezone’s localize
method instead of replace(tzinfo=tz). When extracted.deadline is date-only,
interpret it as 23:59:59 in the user’s timezone before comparing with now;
preserve timezone-aware inputs and the existing invalid-date response.
In `@apps/intelligence/app/services/prometheus.py`:
- Around line 592-607: Thread the computed request ID through
get_portfolio_insights, get_market_sentiment, get_yield_recommendation, and
get_vault_recommendations, passing it to every strip_system_prompt_leakage call
in those functions. Update their analyze.py router callers to compute and supply
_request_id(request), preserving consistent audit correlation across all Claude
response paths.
---
Nitpick comments:
In `@apps/intelligence/app/routers/analyze.py`:
- Around line 52-55: Promote analyze.py’s _request_id into a shared router
utility, preserving its existing middleware-backed fallback behavior, and import
it wherever request correlation IDs are needed. Update
apps/intelligence/app/routers/analyze.py lines 52-55 to define or import the
shared helper, replace the inline expression at
apps/intelligence/app/routers/chat.py line 48 with that helper, and replace the
inline expression at apps/intelligence/app/routers/coaching.py line 27 likewise.
In `@apps/intelligence/app/services/claude.py`:
- Around line 1-11: Merge the standalone “Claude client configuration” docstring
with the following module documentation into one module-level docstring,
preserving the full description and ensuring it is the first statement in the
module so the combined text becomes __doc__.
- Around line 20-29: Update get_model_id() to read and return
settings.anthropic_model at call time instead of returning the import-time
MODEL_ID constant; remove the frozen MODEL_ID indirection so runtime settings
reloads and model-pin checks observe the configured value.
In `@apps/intelligence/app/services/guardrails.py`:
- Around line 177-182: Update `_wrap` to neutralize fake opening and closing
tags using case-insensitive regex matching that permits whitespace inside the
tag syntax, rather than exact-literal replacement. Preserve truncation, the
surrounding wrapper tags, and existing behavior for valid content; apply the
same neutralization to both opening and closing forms.
In `@apps/intelligence/app/services/prometheus.py`:
- Around line 418-446: Update the streaming buffer logic around
strip_system_prompt_leakage so pending is replaced with the sanitized value even
when emit_len is non-positive. Preserve the existing yield behavior for positive
emit_len, while ensuring subsequent iterations never retain the unsanitized
pending content.
🪄 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: 97a2f792-7bf9-4ad6-9767-419f5c7d1ad7
⛔ Files ignored due to path filters (2)
apps/api/go.sumis excluded by!**/*.sumpackages/contracts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
AUDIT_REPORT.mdAUDIT_THREAT_MODEL.mdCI_CD_VALIDATION_REPORT.mdSEP24_DECISION.mdapps/api/go.modapps/intelligence/app/main.pyapps/intelligence/app/models/coaching.pyapps/intelligence/app/models/recommendation.pyapps/intelligence/app/routers/analyze.pyapps/intelligence/app/routers/chat.pyapps/intelligence/app/routers/coaching.pyapps/intelligence/app/routers/natural_language_goal.pyapps/intelligence/app/routers/ws_chat.pyapps/intelligence/app/services/claude.pyapps/intelligence/app/services/goal_extractor.pyapps/intelligence/app/services/guardrails.pyapps/intelligence/app/services/prometheus.pyapps/intelligence/tests/test_goal_extractor.pyapps/intelligence/tests/test_guardrails.py
💤 Files with no reviewable changes (4)
- SEP24_DECISION.md
- CI_CD_VALIDATION_REPORT.md
- AUDIT_THREAT_MODEL.md
- AUDIT_REPORT.md
Comment on lines
+100
to
+105
| # TODO: Replace with actual Go service call via relay | ||
| return { | ||
| "success": True, | ||
| "message": "Goal validated successfully", | ||
| "goal_id": f"goal_{datetime.now().strftime('%Y%m%d_%H%M%S')}", | ||
| "goal": result.extracted.model_dump() if result.extracted else goal_data, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report a goal as created before calling the validated creation service.
This returns a fabricated goal_id and success response without persisting anything. It also bypasses the existing goal-creation service required by this feature’s contract. Call the relay/backend service and return its actual created goal instead.
🤖 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 `@apps/intelligence/app/routers/natural_language_goal.py` around lines 100 -
105, The natural-language goal flow currently fabricates success in the TODO
return block without creating a goal. Replace this response in the surrounding
goal-validation handler with a call to the existing relay/backend validated
creation service, pass the validated goal data, and return the service’s actual
creation result and goal identifier.
Comment on lines
+111
to
+113
| except Exception as e: | ||
| logger.error(f"Goal creation failed: {e}") | ||
| raise HTTPException(status_code=500, detail="Failed to create goal.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve intended validation errors.
The HTTPException(400) raised on Lines 94-98 is caught here as a generic Exception and returned as a 500. Re-raise HTTPException before this handler.
Proposed fix
+ except HTTPException:
+ raise
except ValueError as e:
logger.error(f"Validation error: {e}")
raise HTTPException(status_code=400, detail=f"Invalid data: {str(e)}")🤖 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 `@apps/intelligence/app/routers/natural_language_goal.py` around lines 111 -
113, Update the exception handling around goal creation so HTTPException
instances, including the intentional 400 validation error, are re-raised
unchanged before the generic Exception handler. Keep logging and converting
unexpected exceptions to the existing 500 response in the natural-language goal
creation flow.
Comment on lines
+64
to
+70
| response = self.client.messages.create( | ||
| model=self.model, | ||
| max_tokens=1024, | ||
| messages=[{"role": "user", "content": self._build_extraction_prompt(user_message)}], | ||
| tools=[{"name": "extract_goal", "input_schema": ExtractedGoal.model_json_schema()}], | ||
| tool_choice={"type": "tool", "name": "extract_goal"}, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
model=self.model fails the repo's pinned-model regression test.
tests/test_guardrails.py:389-404 flags any model= value that isn't settings.anthropic_model (or containing anthropic_model) when max_tokens is nearby. self.model matches that heuristic, so this call site will be reported as an offender. Either reference the setting directly here or rename the attribute (e.g. self.anthropic_model).
🐛 Keep the pin visible at the call site
- def __init__(self):
- self.client = get_client()
- self.model = get_model_id()
+ def __init__(self):
+ self.client = get_client()
+ self.anthropic_model = get_model_id() response = self.client.messages.create(
- model=self.model,
+ model=self.anthropic_model,
max_tokens=1024,📝 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
| response = self.client.messages.create( | |
| model=self.model, | |
| max_tokens=1024, | |
| messages=[{"role": "user", "content": self._build_extraction_prompt(user_message)}], | |
| tools=[{"name": "extract_goal", "input_schema": ExtractedGoal.model_json_schema()}], | |
| tool_choice={"type": "tool", "name": "extract_goal"}, | |
| ) | |
| response = self.client.messages.create( | |
| model=self.anthropic_model, | |
| max_tokens=1024, | |
| messages=[{"role": "user", "content": self._build_extraction_prompt(user_message)}], | |
| tools=[{"name": "extract_goal", "input_schema": ExtractedGoal.model_json_schema()}], | |
| tool_choice={"type": "tool", "name": "extract_goal"}, | |
| ) |
🤖 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 `@apps/intelligence/app/services/goal_extractor.py` around lines 64 - 70,
Update the messages.create call in the goal extraction flow to pass
settings.anthropic_model directly as the model value, or rename and use the
instance attribute to clearly include anthropic_model. Keep the pinned model
visible at this call site so the guardrail regression test recognizes it.
Comment on lines
+83
to
+111
| def _check_injection(self, message: str) -> bool: | ||
| patterns = [ | ||
| r"ignore (?:the|all) (?:previous|above) (?:instructions?|prompt)", | ||
| r"(?:system|developer|assistant).*(?:prompt|instruction)", | ||
| r"you are (?:now|not)", | ||
| r"pretend (?:you|to be)", | ||
| r"role[- ]?play", | ||
| r"act as", | ||
| r"forget (?:all|previous|above)", | ||
| r"disregard", | ||
| r"do not (?:follow|obey|listen to)", | ||
| r"you must (?:now|not)", | ||
| r"new rules?", | ||
| r"override", | ||
| ] | ||
| lower_msg = message.lower() | ||
| for pattern in patterns: | ||
| if re.search(pattern, lower_msg): | ||
| return True | ||
| return False | ||
|
|
||
| def _build_extraction_prompt(self, message: str) -> str: | ||
| categories = ", ".join(self.CATEGORIES) | ||
| return f"""Extract a structured savings goal from the user's message. | ||
| Current date: {datetime.now().strftime("%Y-%m-%d")} | ||
| User message: "{message}" | ||
| Extract: name, target_amount (USDC), deadline (YYYY-MM-DD), category [{categories}], | ||
| initial_deposit, is_recurring, recurring_amount. | ||
| Rules: Do NOT guess missing fields. Return ONLY the structured extraction.""" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reuse guardrails instead of a second, coarser screener; wrap the user message in the trust boundary.
Two problems in the same path:
_check_injectionduplicatesguardrails.screen_inputwith far broader patterns — baredisregard,override,act as,new rules?,role[- ]?playwill reject legitimate goal descriptions ("override my old savings plan", "save for a role-play convention").messageis interpolated raw into the prompt with noguardrails.wrap_user_content, so this endpoint is the one Claude path in the service without an explicit instruction/data boundary.
♻️ Delegate to the guardrails module
-import re
from datetime import datetime
from typing import Optional
import pytz
from pydantic import BaseModel, Field
+from app.services import guardrails
from app.services.claude import get_client, get_model_id- def extract(self, user_message: str, user_timezone: str = "UTC") -> GoalExtractionResult:
- if self._check_injection(user_message):
- return GoalExtractionResult(success=False, error="Invalid input detected.")
+ def extract(
+ self,
+ user_message: str,
+ user_timezone: str = "UTC",
+ *,
+ request_id: str = "",
+ user_id: str = "",
+ ) -> GoalExtractionResult:
+ if guardrails.screen_input(
+ user_message, request_id=request_id, user_id=user_id
+ ).flagged:
+ return GoalExtractionResult(success=False, error=guardrails.REFUSAL_MESSAGE)
+ user_message = guardrails.truncate_message(user_message)-User message: "{message}"
+{guardrails.wrap_user_content(message)}🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 99-99: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(pattern, lower_msg)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 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 `@apps/intelligence/app/services/goal_extractor.py` around lines 83 - 111,
Replace the bespoke `_check_injection` pattern scan with the existing
`guardrails.screen_input` behavior, avoiding duplicate broader filtering and
preserving legitimate goal descriptions. In `_build_extraction_prompt`, wrap the
user message with `guardrails.wrap_user_content` before interpolation so it is
explicitly separated from the extraction instructions.
Comment on lines
+104
to
+111
| def _build_extraction_prompt(self, message: str) -> str: | ||
| categories = ", ".join(self.CATEGORIES) | ||
| return f"""Extract a structured savings goal from the user's message. | ||
| Current date: {datetime.now().strftime("%Y-%m-%d")} | ||
| User message: "{message}" | ||
| Extract: name, target_amount (USDC), deadline (YYYY-MM-DD), category [{categories}], | ||
| initial_deposit, is_recurring, recurring_amount. | ||
| Rules: Do NOT guess missing fields. Return ONLY the structured extraction.""" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"Current date" ignores the user's timezone, so relative dates resolve incorrectly.
datetime.now() is server-local and naive, while validation uses user_timezone. "by next Friday" from a UTC+13 user near midnight resolves off by a day, and can then be rejected as past.
🐛 Anchor the prompt date in the user's zone
- def _build_extraction_prompt(self, message: str) -> str:
+ def _build_extraction_prompt(self, message: str, user_timezone: str = "UTC") -> str:
categories = ", ".join(self.CATEGORIES)
+ try:
+ tz = pytz.timezone(user_timezone)
+ except pytz.UnknownTimeZoneError:
+ tz = pytz.UTC
return f"""Extract a structured savings goal from the user's message.
-Current date: {datetime.now().strftime("%Y-%m-%d")}
+Current date: {datetime.now(tz).strftime("%Y-%m-%d")} (timezone: {tz})📝 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 _build_extraction_prompt(self, message: str) -> str: | |
| categories = ", ".join(self.CATEGORIES) | |
| return f"""Extract a structured savings goal from the user's message. | |
| Current date: {datetime.now().strftime("%Y-%m-%d")} | |
| User message: "{message}" | |
| Extract: name, target_amount (USDC), deadline (YYYY-MM-DD), category [{categories}], | |
| initial_deposit, is_recurring, recurring_amount. | |
| Rules: Do NOT guess missing fields. Return ONLY the structured extraction.""" | |
| def _build_extraction_prompt(self, message: str, user_timezone: str = "UTC") -> str: | |
| categories = ", ".join(self.CATEGORIES) | |
| try: | |
| tz = pytz.timezone(user_timezone) | |
| except pytz.UnknownTimeZoneError: | |
| tz = pytz.UTC | |
| return f"""Extract a structured savings goal from the user's message. | |
| Current date: {datetime.now(tz).strftime("%Y-%m-%d")} (timezone: {tz}) | |
| User message: "{message}" | |
| Extract: name, target_amount (USDC), deadline (YYYY-MM-DD), category [{categories}], | |
| initial_deposit, is_recurring, recurring_amount. | |
| Rules: Do NOT guess missing fields. Return ONLY the structured extraction.""" |
🤖 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 `@apps/intelligence/app/services/goal_extractor.py` around lines 104 - 111,
Update _build_extraction_prompt to derive the displayed current date using the
user's configured user_timezone rather than naive server-local datetime.now().
Preserve the existing YYYY-MM-DD formatting so relative-date extraction and
subsequent validation use the same user-local calendar date.
Comment on lines
+126
to
+153
| try: | ||
| # Use user's timezone | ||
| try: | ||
| tz = pytz.timezone(user_timezone) | ||
| except pytz.UnknownTimeZoneError: | ||
| tz = pytz.UTC | ||
|
|
||
| now = datetime.now(tz) | ||
| deadline_date = datetime.fromisoformat(extracted.deadline).replace(tzinfo=tz) | ||
|
|
||
| if deadline_date < now: | ||
| return GoalExtractionResult( | ||
| success=False, | ||
| ambiguity=AmbiguityResponse( | ||
| is_ambiguous=True, | ||
| message=f"The date {extracted.deadline} is in the past.", | ||
| missing_fields=["deadline"], | ||
| ), | ||
| ) | ||
| except ValueError: | ||
| return GoalExtractionResult( | ||
| success=False, | ||
| ambiguity=AmbiguityResponse( | ||
| is_ambiguous=True, | ||
| message=f"Could not understand the date '{extracted.deadline}'.", | ||
| missing_fields=["deadline"], | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
pytz localize vs replace tzinfo wrong LMT offset
💡 Result:
Using pytz with the standard datetime.replace(tzinfo=...) method is technically incorrect and causes issues because pytz timezone objects do not follow the standard Python tzinfo interface [1][2]. Specifically, when you attach a pytz timezone object via replace, it defaults to the first historical offset in that zone's database, which is typically the Local Mean Time (LMT) [3][4][5]. LMT was used before the standardization of time zones and often contains non-standard, minute-level offsets (e.g., -00:53) that are rarely the intended result [3][6][4]. The correct way to create a localized datetime object with pytz is to use the.localize method provided by the timezone object [7][4][1]. Unlike replace, which simply attaches the tzinfo object without any calculation [8][9], the localize method actively calculates which specific UTC offset was in effect at the given local time in that geographical location, ensuring the correct offset (such as CET or EDT) is applied [3][6][1]. In summary: - Do not use datetime.replace(tzinfo=pytz_timezone) [8][5][9]. - Use pytz_timezone.localize(naive_datetime) instead [7][1][10]. - If you perform arithmetic on a pytz-aware datetime, you must use.normalize to correct the offset if the calculation crosses Daylight Saving Time boundaries, as pytz does not automatically update the offset in response to arithmetic [6][1][2]. Due to these complexities, it is highly recommended to perform all internal date and time operations in UTC and only convert to local time zones at the edges of your application (e.g., for display or I/O) [1][8][2]. If you are using modern Python (3.9+), consider using the standard library's zoneinfo module, which is designed to be compatible with the standard tzinfo interface and avoids these common pytz pitfalls [11][6][10].
Citations:
- 1: https://pypi.org/project/pytz/
- 2: https://pythonhosted.org/pytz/
- 3: https://mail.python.org/pipermail/python-list/2016-June/860571.html
- 4: https://peter.demin.dev/12_articles/69-pytz.html
- 5: https://stackoverflow.com/questions/63595789/incorrect-datetime-conversion-from-america-new-york-time-to-utc-and-back-to-amer
- 6: https://blog.ganssle.io/articles/2018/03/pytz-fastest-footgun.html
- 7: https://stackoverflow.com/questions/35631578/python-datetime-pytz-issue
- 8: https://stackoverflow.com/questions/1379740/pytz-localize-vs-datetime-replace
- 9: https://stackoverflow.com/questions/51741177/python-datetime-difference-between-tz-inside-method-and-replacetzinfo
- 10: https://stackoverflow.com/questions/45242194/is-datetime-replace-fundamentally-broken
- 11: https://stackoverflow.com/questions/66320919/why-pytz-localize-and-datetime-replace-return-different-results
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant region with line numbers.
file="apps/intelligence/app/services/goal_extractor.py"
wc -l "$file"
sed -n '1,240p' "$file" | cat -n
# Search for related deadline/date handling and tests.
rg -n "deadline|fromisoformat|localize|replace\\(tzinfo|is_ambiguous|missing_fields" apps/intelligence -SRepository: Suncrest-Labs/nester
Length of output: 17977
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/intelligence/tests/test_goal_extractor.py"
wc -l "$file"
sed -n '1,220p' "$file" | cat -nRepository: Suncrest-Labs/nester
Length of output: 3777
Localize the parsed deadline and treat date-only input as end-of-day datetime.fromisoformat(...).replace(tzinfo=tz) attaches a pytz zone incorrectly, and with a YYYY-MM-DD deadline it makes same-day goals read as past as soon as the day starts. Use tz.localize() for naive values and compare date-only deadlines at 23:59:59 in the user's timezone.
🤖 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 `@apps/intelligence/app/services/goal_extractor.py` around lines 126 - 153,
Update the deadline parsing in the goal extraction validation block to localize
naive datetimes with the pytz timezone’s localize method instead of
replace(tzinfo=tz). When extracted.deadline is date-only, interpret it as
23:59:59 in the user’s timezone before comparing with now; preserve
timezone-aware inputs and the existing invalid-date response.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (5)
apps/intelligence/app/services/prometheus.py (1)
418-446: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBuffer isn't updated to the sanitized value when
emit_len <= 0.When
strip_system_prompt_leakageshrinkspendingenough thatemit_len <= 0,pendingkeeps its original unsanitized content instead of being set tosanitized. Nothing unsafe reaches the client (everyyieldusessanitized), and the end-of-stream flush re-sanitizes anyway, so this is a very narrow, low-impact edge case rather than a real leak.♻️ Optional tightening
sanitized = guardrails.strip_system_prompt_leakage( pending, request_id=request_id ) emit_len = len(sanitized) - _LEAK_OVERLAP if emit_len > 0: safe_chunk = sanitized[:emit_len].replace("\n", "\\n") yield f"data: {safe_chunk}\n\n" pending = sanitized[emit_len:] + else: + pending = sanitized🤖 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 `@apps/intelligence/app/services/prometheus.py` around lines 418 - 446, Update the streaming buffer logic around strip_system_prompt_leakage so pending is replaced with the sanitized value even when emit_len is non-positive. Preserve the existing yield behavior for positive emit_len, while ensuring subsequent iterations never retain the unsanitized pending content.apps/intelligence/app/routers/analyze.py (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated request-id fallback logic across three routers.
analyze.py,chat.py, andcoaching.pyeach independently derive a request-correlation id with the samegetattr(request.state, "request_id", "") or str(uuid.uuid4())expression; a shared helper (e.g. in a common router-utils module) would keep this single-sourced now thatadd_request_idmiddleware inmain.pyis the actual source of truth.
apps/intelligence/app/routers/analyze.py#L52-L55: promote_request_idinto a shared utility importable by the other routers.apps/intelligence/app/routers/chat.py#L48-L48: replace the inline expression with the shared helper.apps/intelligence/app/routers/coaching.py#L27-L27: replace the inline expression with the shared helper.🤖 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 `@apps/intelligence/app/routers/analyze.py` around lines 52 - 55, Promote analyze.py’s _request_id into a shared router utility, preserving its existing middleware-backed fallback behavior, and import it wherever request correlation IDs are needed. Update apps/intelligence/app/routers/analyze.py lines 52-55 to define or import the shared helper, replace the inline expression at apps/intelligence/app/routers/chat.py line 48 with that helper, and replace the inline expression at apps/intelligence/app/routers/coaching.py line 27 likewise.apps/intelligence/app/services/claude.py (2)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo module docstrings — the second is a dead string expression.
Only line 1 becomes
__doc__; lines 3-11 are an unused literal. Merge them.♻️ Merge docstrings
-"""Claude client configuration.""" - -"""Claude client and per-user tone/style prompt helpers (`#927`). +"""Claude client configuration and per-user tone/style prompt helpers (`#927`).🤖 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 `@apps/intelligence/app/services/claude.py` around lines 1 - 11, Merge the standalone “Claude client configuration” docstring with the following module documentation into one module-level docstring, preserving the full description and ensuring it is the first statement in the module so the combined text becomes __doc__.
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the model from settings at call time.
MODEL_IDfreezessettings.anthropic_modelat import, so any runtime/reload of settings is ignored, and the indirection also hides the pin from the repo's model-pin regression test (see the related comment ingoal_extractor.py).♻️ Resolve model per call
-# Use the configured model from settings -MODEL_ID = settings.anthropic_model - - def get_client(): return client -def get_model_id(): - return MODEL_ID +def get_model_id() -> str: + return settings.anthropic_model🤖 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 `@apps/intelligence/app/services/claude.py` around lines 20 - 29, Update get_model_id() to read and return settings.anthropic_model at call time instead of returning the import-time MODEL_ID constant; remove the frozen MODEL_ID indirection so runtime settings reloads and model-pin checks observe the configured value.apps/intelligence/app/services/guardrails.py (1)
177-182: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTag stripping in
_wrapis exact-literal only.
replace(close_tag, "")misses whitespace/case variants (</ user_message>,</USER_MESSAGE>).screen_input'sfake_boundary_tagpattern covers the live path, so this is defense-in-depth for callers that wrap without screening (e.g.wrap_context_block).♻️ Regex-based tag neutralisation
-def _wrap(tag: str, content: str, max_chars: int) -> str: - truncated = content[:max_chars] - close_tag = f"</{tag}>" - # Neutralise any attempt to smuggle a fake closing tag to escape the block. - escaped = truncated.replace(close_tag, "").replace(f"<{tag}>", "") - return f"<{tag}>\n{escaped}\n{close_tag}" +def _wrap(tag: str, content: str, max_chars: int) -> str: + truncated = content[:max_chars] + # Neutralise any attempt to smuggle a fake boundary tag (including + # whitespace/case variants) to escape the block. + escaped = re.sub( + rf"</?\s*{re.escape(tag)}\s*>", "", truncated, flags=re.IGNORECASE + ) + return f"<{tag}>\n{escaped}\n</{tag}>"🤖 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 `@apps/intelligence/app/services/guardrails.py` around lines 177 - 182, Update `_wrap` to neutralize fake opening and closing tags using case-insensitive regex matching that permits whitespace inside the tag syntax, rather than exact-literal replacement. Preserve truncation, the surrounding wrapper tags, and existing behavior for valid content; apply the same neutralization to both opening and closing forms.
🤖 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 `@apps/intelligence/app/routers/natural_language_goal.py`:
- Around line 100-105: The natural-language goal flow currently fabricates
success in the TODO return block without creating a goal. Replace this response
in the surrounding goal-validation handler with a call to the existing
relay/backend validated creation service, pass the validated goal data, and
return the service’s actual creation result and goal identifier.
- Around line 111-113: Update the exception handling around goal creation so
HTTPException instances, including the intentional 400 validation error, are
re-raised unchanged before the generic Exception handler. Keep logging and
converting unexpected exceptions to the existing 500 response in the
natural-language goal creation flow.
In `@apps/intelligence/app/services/goal_extractor.py`:
- Around line 83-111: Replace the bespoke `_check_injection` pattern scan with
the existing `guardrails.screen_input` behavior, avoiding duplicate broader
filtering and preserving legitimate goal descriptions. In
`_build_extraction_prompt`, wrap the user message with
`guardrails.wrap_user_content` before interpolation so it is explicitly
separated from the extraction instructions.
- Around line 104-111: Update _build_extraction_prompt to derive the displayed
current date using the user's configured user_timezone rather than naive
server-local datetime.now(). Preserve the existing YYYY-MM-DD formatting so
relative-date extraction and subsequent validation use the same user-local
calendar date.
- Around line 64-70: Update the messages.create call in the goal extraction flow
to pass settings.anthropic_model directly as the model value, or rename and use
the instance attribute to clearly include anthropic_model. Keep the pinned model
visible at this call site so the guardrail regression test recognizes it.
- Around line 126-153: Update the deadline parsing in the goal extraction
validation block to localize naive datetimes with the pytz timezone’s localize
method instead of replace(tzinfo=tz). When extracted.deadline is date-only,
interpret it as 23:59:59 in the user’s timezone before comparing with now;
preserve timezone-aware inputs and the existing invalid-date response.
In `@apps/intelligence/app/services/prometheus.py`:
- Around line 592-607: Thread the computed request ID through
get_portfolio_insights, get_market_sentiment, get_yield_recommendation, and
get_vault_recommendations, passing it to every strip_system_prompt_leakage call
in those functions. Update their analyze.py router callers to compute and supply
_request_id(request), preserving consistent audit correlation across all Claude
response paths.
---
Nitpick comments:
In `@apps/intelligence/app/routers/analyze.py`:
- Around line 52-55: Promote analyze.py’s _request_id into a shared router
utility, preserving its existing middleware-backed fallback behavior, and import
it wherever request correlation IDs are needed. Update
apps/intelligence/app/routers/analyze.py lines 52-55 to define or import the
shared helper, replace the inline expression at
apps/intelligence/app/routers/chat.py line 48 with that helper, and replace the
inline expression at apps/intelligence/app/routers/coaching.py line 27 likewise.
In `@apps/intelligence/app/services/claude.py`:
- Around line 1-11: Merge the standalone “Claude client configuration” docstring
with the following module documentation into one module-level docstring,
preserving the full description and ensuring it is the first statement in the
module so the combined text becomes __doc__.
- Around line 20-29: Update get_model_id() to read and return
settings.anthropic_model at call time instead of returning the import-time
MODEL_ID constant; remove the frozen MODEL_ID indirection so runtime settings
reloads and model-pin checks observe the configured value.
In `@apps/intelligence/app/services/guardrails.py`:
- Around line 177-182: Update `_wrap` to neutralize fake opening and closing
tags using case-insensitive regex matching that permits whitespace inside the
tag syntax, rather than exact-literal replacement. Preserve truncation, the
surrounding wrapper tags, and existing behavior for valid content; apply the
same neutralization to both opening and closing forms.
In `@apps/intelligence/app/services/prometheus.py`:
- Around line 418-446: Update the streaming buffer logic around
strip_system_prompt_leakage so pending is replaced with the sanitized value even
when emit_len is non-positive. Preserve the existing yield behavior for positive
emit_len, while ensuring subsequent iterations never retain the unsanitized
pending content.
🪄 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: 97a2f792-7bf9-4ad6-9767-419f5c7d1ad7
⛔ Files ignored due to path filters (2)
apps/api/go.sumis excluded by!**/*.sumpackages/contracts/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
AUDIT_REPORT.mdAUDIT_THREAT_MODEL.mdCI_CD_VALIDATION_REPORT.mdSEP24_DECISION.mdapps/api/go.modapps/intelligence/app/main.pyapps/intelligence/app/models/coaching.pyapps/intelligence/app/models/recommendation.pyapps/intelligence/app/routers/analyze.pyapps/intelligence/app/routers/chat.pyapps/intelligence/app/routers/coaching.pyapps/intelligence/app/routers/natural_language_goal.pyapps/intelligence/app/routers/ws_chat.pyapps/intelligence/app/services/claude.pyapps/intelligence/app/services/goal_extractor.pyapps/intelligence/app/services/guardrails.pyapps/intelligence/app/services/prometheus.pyapps/intelligence/tests/test_goal_extractor.pyapps/intelligence/tests/test_guardrails.py
💤 Files with no reviewable changes (4)
- SEP24_DECISION.md
- CI_CD_VALIDATION_REPORT.md
- AUDIT_THREAT_MODEL.md
- AUDIT_REPORT.md
🛑 Comments failed to post (1)
apps/intelligence/app/services/prometheus.py (1)
592-607: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Thread
request_idthrough the remaining Claude call sites for consistent audit correlation.Unlike every other function touched in this diff,
get_portfolio_insights,get_market_sentiment,get_yield_recommendation, andget_vault_recommendationscallstrip_system_prompt_leakagewithout arequest_id, so any leak-detection warning from these paths logsrequest_id=unknown. Their router callers inanalyze.pyalready compute_request_id(request)for other endpoints in this same PR, but don't pass it to these four.♻️ Suggested fix (repeat for the other three functions)
-async def get_portfolio_insights(user_id: str) -> list[dict[str, Any]]: +async def get_portfolio_insights( + user_id: str, request_id: str = "" +) -> list[dict[str, Any]]: ... - card[field] = guardrails.strip_system_prompt_leakage( - card[field] - ) + card[field] = guardrails.strip_system_prompt_leakage( + card[field], request_id=request_id + )Also applies to: 641-646, 779-784, 827-837
🤖 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 `@apps/intelligence/app/services/prometheus.py` around lines 592 - 607, Thread the computed request ID through get_portfolio_insights, get_market_sentiment, get_yield_recommendation, and get_vault_recommendations, passing it to every strip_system_prompt_leakage call in those functions. Update their analyze.py router callers to compute and supply _request_id(request), preserving consistent audit correlation across all Claude response paths.
* test(contracts): add negative authorization coverage * test: tighten negative authorization assertions --------- Co-authored-by: Deon <110722148+0xDeon@users.noreply.github.com>
… cipher (Suncrest-Labs#799) * feat(crypto): version-tagged envelope cipher for account numbers Replace the single-key AccountCipher with a multi-version AES-256-GCM cipher. Encrypt seals with the active key and returns a CipherEnvelope carrying the key version; Decrypt resolves the key by the ciphertext's version and fails with ErrUnknownKeyVersion when it is not registered. NewAccountCipher is retained (registers the key as v1) for backward compatibility. Fingerprints use a stable pepper independent of the active key so the uniqueness index survives rotation. * test(crypto): cover active-key encrypt, cross-version decrypt, unknown version, fingerprint stability * feat(config): add AccountCipherConfig accessor and versioned key set type * feat(config): parse ACCOUNT_CIPHER_KEYS/ACTIVE_KEY with legacy single-key fallback ACCOUNT_CIPHER_KEYS (comma-separated version:base64 pairs) plus ACCOUNT_CIPHER_ACTIVE_KEY take precedence; when unset, the existing BANK_ACCOUNT_ENCRYPTION_KEY is registered as v1 so single-key deployments keep working. Validates active version membership and pair format at startup. * test(config): cover multi-key parsing, legacy fallback, and validation errors * docs(config): document account cipher key set and rotation env vars in .env.example * feat(db): add bank_accounts.key_version column defaulting existing rows to v1 New column records which key sealed each row so rotation never rewrites history. Indexed so the rotation tool can cheaply find un-rotated rows. * feat(db): down migration dropping key_version column and index * feat(bankaccount): thread key version through Repository Create/GetByID * feat(repo): persist key_version on bank account insert * feat(repo): return key_version from GetByID and add rotation Store methods GetByID now yields the stored key version so callers can decrypt with the right key. CountPending/ScanPending/UpdateCipher implement rotation.Store for the rotation tool; UpdateCipher leaves the fingerprint untouched so the uniqueness index is unaffected. * feat(rotation): idempotent, resumable batch key-rotation engine Rotator scans rows not on the active key version, decrypts each with its recorded version, re-seals with the active key, and commits per row. A second run finds nothing (idempotent); an interrupted run resumes from the remainder. Logs only counts and row IDs, never plaintext, keys, or ciphertext. * test(rotation): re-encrypt to active, idempotency, resume-after-interrupt, no data loss * feat(service): seal new accounts with the active key envelope * feat(service): decrypt saved accounts by their stored key version ResolveForSettlement reconstructs the CipherEnvelope from the stored ciphertext and key version; SetDefault/Remove absorb the extra GetByID return value. * test(service): update in-memory repo mock for key-versioned signatures * test(service): new writes use active key; legacy row decrypts after key added * feat(cmd): rotate_keys CLI to re-encrypt accounts onto the active key Loads the same key config as the API, refuses to run when no cipher is configured, and drives the rotation engine over the bank_accounts store with -batch-size and -timeout flags. * feat(api): wire the multi-key account cipher from AccountCipherConfig * docs(security): key versioning model, env format, and 5-step rotation runbook * fix(crypto): require explicit fingerprint pepper when no v1 key is configured Defaulting the fingerprint pepper to the active key let it change on every rotation (e.g. v2->v3) and silently break blind-index uniqueness. Fail closed with ErrFingerprintKeyRequired instead; cover the no-v1 active-key rotation case. (CodeRabbit) * fix(config): fail closed on empty keyset, over-long versions, and v1-less sets - ACCOUNT_CIPHER_KEYS that parses to zero entries now errors instead of silently disabling the cipher, and an active version absent from the set is always rejected. - Reject key versions longer than 32 chars (bank_accounts.key_version is VARCHAR(32)) before they fail at the DB boundary. - Require ACCOUNT_CIPHER_FINGERPRINT_KEY when the key set has no v1. (CodeRabbit) * fix(db): guard key_version rollback and build its index concurrently - 057 down aborts if any row is on a non-v1 key, since dropping key_version would make rotated ciphertext undecryptable. - Move the index into 058 using CREATE INDEX CONCURRENTLY so a large bank_accounts table is not write-locked during deploy. (CodeRabbit) * docs: clarify that a v1-less key set must set an explicit fingerprint pepper (CodeRabbit) * test(service): consolidate key-versioning scenarios into a table-driven test (CodeRabbit) * docs: clarify no active-key fallback; config-load failure vs constructor sentinel (CodeRabbit)
…st-Labs#800) * feat(api): add distributed rate limiting with strict route limits Extend the existing in-memory rate limiter with a dual-mode backend: a Redis fixed-window counter for cross-instance enforcement, falling back to the in-memory token bucket when REDIS_ADDR is unset. - Add Limiter interface + NewLimiter factory (Redis or in-memory) - Global per-IP limiter now excludes /health*, /readyz, /metrics - Strict per-IP limiter on POST /auth/challenge and /auth/verify (credential stuffing) and strict per-user limiter on POST /settlements (settlement spam) - New RATELIMIT_AUTH_* and RATELIMIT_SETTLEMENT_* config knobs + .env - Redis limiter fails open on outage so it never blocks live traffic - Table-driven tests: under/over limit, 429 + Retry-After, window reset, per-IP and per-user isolation, memory fallback, and a Redis integration test guarded by REDIS_ADDR * fix(api): address CodeRabbit review on rate limiting - CORS: move cors middleware outermost so 429 responses from the global and auth-route limiters still carry Access-Control-Allow-Origin and stay readable to browser clients - Redis: bound each limiter round-trip with a 75ms timeout so a slow (not just down) Redis fails fast into fail-open instead of adding multi-second latency to every request; log fail-open events - Proxy-aware client IP: add RATELIMIT_TRUSTED_PROXY_COUNT (default 0). When set, derive the client IP from X-Forwarded-For counting hops from the right, so traffic behind a load balancer keys off the real client instead of collapsing onto the proxy address, without letting clients spoof past the trusted-proxy boundary - Tests: proxy-aware keying + spoof resistance, and config default / override / negative-validation for the new knob * fix(api): reject sub-millisecond rate-limit windows The Redis limiter converts the window to whole milliseconds for PEXPIRE, so a positive but sub-1ms window (e.g. 500us) truncates to 0, expiring the counter immediately and silently disabling enforcement. Reject global/auth/settlement windows below 1ms at config load, with a regression test.
…olio valuation, RAG grounding (Suncrest-Labs#824 Suncrest-Labs#845 Suncrest-Labs#832 Suncrest-Labs#852) (Suncrest-Labs#876) * feat(api): durable async job queue (Suncrest-Labs#824) PostgreSQL-backed job queue with FOR UPDATE SKIP LOCKED dequeue, lease-based visibility timeout with crash recovery, exponential backoff + full jitter, dead-letter queue, per-job-type concurrency limits, idempotent enqueue, and graceful drain on shutdown. Queue-depth/DLQ/latency metrics and correlation-ID propagation included. Worker pool wired into the API with config knobs. * feat(api): yield harvest orchestration engine (Suncrest-Labs#845) Cadence + event-triggered engine that applies the economic gate (harvest iff accrued yield > gas fee + margin), defers under network congestion, and submits harvests as idempotent, window-deduplicated jobs on the Suncrest-Labs#824 queue. Includes a gas oracle abstraction, vault/user/service adapters, an idempotent job handler, and an owner-scoped harvest-status API (pending yield, threshold, estimated next harvest). Pure decision core and engine fully unit-tested. * feat(api): real-time portfolio valuation service (Suncrest-Labs#832) Stroop-exact aggregation of positions, pending deposits, accrued yield, goal allocations, and claimable rewards with a structured per-vault/per-goal breakdown (principal vs yield, locked vs flexible, settled vs pending, claimable). Multi-asset oracle pricing with confidence propagation, per-user cache with event-driven invalidation on confirmed transactions, and WebSocket push of refreshed valuations. Pure aggregator, cache, and service unit-tested. * feat(intelligence): RAG grounding for Prometheus AI (Suncrest-Labs#852) Structured retrieval layer that routes queries to the right user-scoped data sources (positions, goals, transactions, yield landscape) without embeddings, assembling only the minimal context needed with citations. Grounding rules force the model to answer solely from retrieved context, cite it, and refuse when data is missing; post-generation numeric validation flags any figure not present in the context to catch hallucinations. Strict per-user isolation: scope is fixed by the JWT subject and cannot be widened by prompt injection. Wired into streaming chat (and WebSocket chat via the shared path). Fully unit-tested.
…eld cache warming (Suncrest-Labs#874) * fix(savingsgoal): soft-archive goals on DELETE instead of hard-delete (Suncrest-Labs#685) Replace the permanent DELETE with an UPDATE that stamps archived_at and sets status to 'archived'. Adds migration 059 to introduce the archived_at column. Already-archived goals surface as ErrGoalNotFound (404) so callers get a sensible response without a 500. Adds two unit tests via sqlmock asserting the soft-delete and already-archived cases. * feat(yield): warm DeFiLlama Stellar cache on service startup (Suncrest-Labs#667) * fix(savingsgoal): validate target_amount and goal name (Suncrest-Labs#692 Suncrest-Labs#681) Suncrest-Labs#692 — Add savingsgoal.ErrInvalidAmount (defined in the savingsgoal domain, not imported from vault) and update validateSavingsGoalInput to return it when target_amount is zero, negative, or below MinTargetAmount (0.01). Handler writeError now maps ErrInvalidAmount to 400 Bad Request, fixing the 500 that was returned when vault.ErrInvalidAmount was not recognised. Suncrest-Labs#681 — Add validateGoalName capping name at MaxGoalNameLength (100 chars) consistent with the savings_goals.name column width. Called on both Create and Update paths so over-long names return a 400 instead of a DB error. * test(savingsgoal): cover amount and name validation cases (Suncrest-Labs#692 Suncrest-Labs#681)
…uncrest-Labs#811) (Suncrest-Labs#879) Introduce shared temporary-storage reentrancy guards and callee allowlists in libs/common, apply them across vault, treasury, and allocation strategy fund-moving paths, and add hostile mocks with adversarial integration tests plus documented resource costs.
Suncrest-Labs#872) * chore(security): fix IDOR vulnerabilities and harden JWT configuration Addresses highest-priority findings from security assessment (Issue Suncrest-Labs#589): Fixed: - Added ownership validation for vault retrieval endpoints (GET /vaults/{id}, GET /vaults/{id}/allocations) - Added ownership validation for transaction creation (POST /transactions) - Added ownership validation for transaction retrieval (GET /transactions/{hash}) - Hardened Intelligence service by preventing production startup without JWT secret via Pydantic model_validator Documentation: - Added docs/security/threat-model.md (assets, trust boundaries, entry points, threat actors, controls) - Added docs/security/pentest-report-v1.md (11 findings with evidence, impact, root cause, remediation, verification) Authorization was implemented at the handler level rather than the service layer because the service methods are shared by numerous trusted internal system components (scheduler, rebalance, TVL, projections). Refactoring those interfaces would have expanded scope considerably and increased regression risk. Closes Suncrest-Labs#589 * style: fix ruff line length in test_config.py * fix: return 404 for missing vault in transaction ownership check * chore: address CodeRabbit review comments - Remove dead var _ = decimal.Zero - Fix function name extractClientIP -> clientIP in evidence - Update Go test result from manual to ALL PASS (CI confirmed) - Correct deposit flow wording (price_per_share is not user-supplied) - Fix WebSocket nil-authenticator mitigation claim (no nil check exists)
Two migrations shared the prefix 059. `059_create_jobs` landed first in Suncrest-Labs#876; `059_add_savings_goal_archived_at` landed eight minutes later in Suncrest-Labs#874 and collided with it. The consequence was worse than a lint failure. golang-migrate refuses to load a directory containing duplicate versions, so migrations could not run at all past 058, and the migration-prefix guard in the API (Go) workflow failed on every pull request touching the Go API — six open PRs were red through no fault of their authors. Renumber the later arrival to 060 and leave `059_create_jobs` in place, since it merged first and is the version any environment already sitting at 59 will have applied. Renumbering it instead would have desynced those environments. `059_add_savings_goal_archived_at` has never been applied anywhere, because the collision prevented golang-migrate from loading the directory in the first place, so moving it is safe. The only in-repo reference to either filename is in job_repository_integration_test.go, which points at `059_create_jobs.up.sql` and is unaffected. Open PRs claiming 060 will need to rebase and renumber.
…ersioning (Suncrest-Labs#882) Implements four platform capabilities: Feature flags (Suncrest-Labs#838) — internal/flags/ - Boolean kill-switch, deterministic percentage rollout, cohort and typed-value flags stored in Postgres (migration 060) - Percentage membership is hash-based and stable: a user in at 10% stays in at 20% - In-process cache with TTL backstop and pub/sub invalidation channel so changes propagate across instances within seconds - Kill switches fail SAFE: evaluator returns the registered safe position when the flag service is unreachable, never fail-open - Secret guard rejects secret-marked names from the flag store - Every change goes through a required AuditRecorder Server-side exports (Suncrest-Labs#839) — internal/export/ - Transaction-history CSV generated from the ledger source of truth with a stable, documented column schema - Reconciliation invariant: exported movements must sum to the ledger's net change per asset or the export errors instead of delivering a wrong document - Exports above a documented row threshold route to the durable job queue - HMAC-signed, time-limited, ownership-verified download tokens; another user cannot fetch someone's export Read-replica routing (Suncrest-Labs#841) — internal/db/router.go - Explicit Read/Write paths declared at call sites, never inferred from SQL - Read-your-writes: users are pinned to the primary for a bounded window after writing (Pinner interface; in-memory impl, Redis-ready) - Unhealthy or lag-exceeding replicas are routed around automatically with primary fallback; transactions always use the primary - Per-role pool stats exposed for metrics; Close drains all pools API versioning (Suncrest-Labs#842) — internal/server/versioning.go, docs/api-versioning.md - Uniform URL-path versioning with versioned route groups - Deprecated versions emit Deprecation, Sunset and successor Link headers on every response; retired versions return 410 Gone with guidance - Unversioned requests route to a pinned default, not 'latest' - Per-version usage counting so retirement is data-driven All packages fully unit-tested (33 tests).
…llocation strategies (Suncrest-Labs#889) Adds a deterministic, constraint-based yield optimizer to the intelligence service (Suncrest-Labs#848). Given candidate yield sources and hard constraints (diversification cap, liquidity floor, lock-horizon fit, risk ceiling, deposit caps, source status), app.services.yield_optimizer.optimize() solves a concave-quadratic risk-adjusted-return objective with scipy.optimize.minimize (SLSQP) and returns per-source weights (fraction and basis points), expected yield, aggregate risk, and a diversification index. Infeasible constraint sets are reported explicitly via infeasibility_reasons and never silently relaxed. The optimizer is a pure, synchronous, dependency-free function, kept separate from app.services.yield_explanation, which has Claude narrate an already-computed result in plain language and validates (via the existing extract_numbers/normalize_number helpers from retrieval.py) that no number absent from the result appears in the explanation, falling back to a deterministic template otherwise. Also wires a new POST /intelligence/yield-optimization endpoint, adds scipy/numpy to requirements.txt, and fixes config.py's stale anthropic_model default (claude-sonnet-4-6 -> claude-sonnet-5). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…d real-time balance endpoints (Suncrest-Labs#891) Co-authored-by: felladaniel36-hash <felladaniel36@gmail.com>
…nd jobs (Suncrest-Labs#894) Adds Postgres-advisory-lock leader election (internal/scheduler/leadership.go) gating all five scheduler background job loops (rebalancer, recurring deposits, APY deviation alerts, goal deadline reminders, protocol health checks) so exactly one instance runs them at a time, with automatic failover bounded by a 3s heartbeat interval and an execution-time leadership recheck immediately before every money-moving/notification action to guard against split-brain during failover. Also fixes a real latent bug the recurring-deposit job had: its transaction hash was derived from the schedule ID alone (constant across every occurrence of a recurring schedule), so only a schedule's first-ever occurrence could ever be recorded — every later occurrence hit vault_transactions' unique transaction_hash constraint and retried forever. The hash now folds in the occurrence timestamp, and the deposit-recording step is routed through the existing durable job queue (internal/domain/ jobqueue) with a per-occurrence idempotency key, mirroring the harvest engine's enqueue pattern, for at-least-once safety. Wires the three job loops that existed but were never started in main.go (the rebalance-decision Scheduler remains unwired pending real on-chain RebalanceSubmitter/YieldFetcher adapters — a pre-existing gap, not a regression), and exposes current leader/instance/since via a new GET /api/v1/admin/scheduler/leadership endpoint. Closes Suncrest-Labs#846 Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…i18n framework (Suncrest-Labs#896) Closes Suncrest-Labs#112, Suncrest-Labs#110, Suncrest-Labs#790, Suncrest-Labs#789 - Savings goal AI coaching (Suncrest-Labs#112): on-demand GET /api/v1/users/savings-goals/{id}/coaching endpoint plus a weekly GoalCoachingScheduler background job, both backed by the existing intelligence /intelligence/coaching endpoint. Progress tracking, on-track status, and required-deposit math already existed in the savingsgoal domain/service; this closes the remaining AI-coaching gap in the issue. - AI rebalancing engine (Suncrest-Labs#110): new risk_model.py (Sharpe-ratio inspired risk_adjusted_score, per-protocol risk factors) and rebalance_engine.py in apps/intelligence, combining live DeFiLlama APY data with a cached-baseline fallback and Claude-generated rationale. New POST /vaults/{id}/rebalance/suggest and /execute endpoints (execute builds an unsigned Stellar transaction via stellar-sdk), proxied through the Go API at the same paths. - PWA installability + offline handling (Suncrest-Labs#790): web app manifest, hand-rolled service worker (app-shell precache, network-first navigation with an /offline fallback, API requests never cached), and an offline guard on the offramp withdraw action. The online/offline hook, banner, and deposit/withdraw guards already existed; this fills the manifest/SW/offline-route/offramp gaps. - i18n framework (Suncrest-Labs#789): locale provider + en/fr message catalogs + a shared formatCurrency/formatNumber/formatDate helper (Intl-based), wired into dashboard/savings/offramp/settings screens, replacing several ad-hoc formatCurrency implementations. Locale persists to localStorage and is selectable from Settings > Preferences. Verified: go build/vet/test + golangci-lint clean; pytest (84 tests) + ruff + mypy --strict clean; vitest (83 tests) + tsc --noEmit + eslint clean. Co-authored-by: Chidimj <Chidimj@users.noreply.github.com>
…tory, referral program (Suncrest-Labs#900) Closes Suncrest-Labs#820, Suncrest-Labs#817, Suncrest-Labs#816, Suncrest-Labs#818 - access_control: granular Role enum (Guardian, Upgrader, Attester, FeeManager, RebalanceKeeper, Treasurer, VaultCreator), generalised two-step role transfer (transfer_role/accept_role/cancel_role_transfer), time-bounded grants (grant_role_until), bounded on-chain enumeration (get_role_members, role_expires_at). Guardian can pause/halt but never unpause/upgrade/withdraw. - vault: autonomous staged circuit breaker (breaker.rs) with independently configurable trip conditions (share-price move, yield sanity, withdrawal velocity with anti-griefing margin, source failure), graded severity (Normal/Throttled/DepositsHalted/FullHalt), staged cooled-down recovery gated to Admin/Upgrader, and an emergency withdrawal path that works at every severity. Guardian-only pause/halt entrypoints added. - vault_factory: new contract deploying vaults from a governed WASM hash via the Soroban deployer, atomic deploy+init, deterministic address prediction, O(1) is_nester_vault registry, bounded pagination, timelocked WASM-hash governance, deprecate_vault. - referral: new standalone contract for a trustless referral program. Rewards accrue from the protocol's performance-fee slice (never the referred user's own yield), gated by minimum deposit/tenure, capped per referrer and by a global budget that halts accrual without clawback. Vault is the sole trusted caller, mirroring the existing treasury.receive_fees pattern. - Narrower roles wired into treasury (Treasurer), yield_registry (Attester), and allocation_strategy/vault (RebalanceKeeper, FeeManager) alongside existing Admin/Operator checks. - EVENTS.md, SECURITY.md, and the contracts README document the new role model, Guardian asymmetry, and breaker/factory/referral event surface. All contracts build to wasm32-unknown-unknown; full workspace test suite and clippy (-D warnings) pass clean.
…#892) * feat(intelligence): add sourced market context signals * fix(intelligence): satisfy market context lint * fix(intelligence): type extraction client boundary * fix(intelligence): harden signal provenance and batching
* feat(security): add adaptive abuse protection * fix(security): harden adaptive abuse state
…ed in user data (Suncrest-Labs#897) * feat(intelligence): personalized savings recommendation engine grounded in user data Adds a savings recommendation engine (app/services/recommendation_engine.py) that generates personalized, actionable recommendations from a user's real goals, positions, and cash-flow behavior. Candidate actions (increase a goal's contribution, move idle balance to higher yield, lock for a term boost, consolidate goals toward the nearest deadline) and every number attached to them are computed deterministically in Python -- Claude, called via tool use, only selects 2-4 candidates, orders them, and writes a short explanation, constrained to a `select_recommendations` tool schema that can only reference candidates by id. A fabrication guard (`_validate_selection`) checks every number in the model's prose against the set of numbers the referenced candidate actually carries, rejects and regenerates once on violation, then falls back to a fully templated explanation built straight from the candidate's own fields -- so a fabricated number can never reach the response. Yield-related candidates always carry risk context (from the vault's real risk score or a documented default disclosure); goal-success figures are integrated with the Suncrest-Labs#843 Monte Carlo simulation endpoint when reachable (two calls -- current vs. proposed contribution -- diffed into a real probability delta) and degrade to a documented heuristic otherwise, since per-user (Redis-backed with an in-memory fallback, mirroring conversation_store.py's pattern) and filtered out permanently; acted-on action types get a deterministic priority boost. Recommendations are cached per-user for 6 hours and invalidated automatically when goal/vault figures change materially, rather than recomputed per page load. Fixes the stale `claude-sonnet-4-6` model id. Closes Suncrest-Labs#847 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(intelligence): correct Suncrest-Labs#843 simulation contract, mypy/ruff cleanup for Suncrest-Labs#847 - projection_client.py: point ProjectionProvider at the real Suncrest-Labs#843 endpoint contract (POST /api/v1/tools/simulation, goal_success.probability) now that it's known, instead of the placeholder GET route guessed before Suncrest-Labs#843's shape was finalized. Computes the success-probability delta from two simulation calls (current vs required contribution) rather than guessing the Go service's internal sensitivity-grid step sizes. - recommendation_engine.py: thread GoalContext into enrich_with_projections so it can build the simulation request; type the Anthropic tool-use call properly (ToolParam/ToolChoiceToolParam/MessageParam) instead of bare dicts, fixing mypy strict errors -- this is the first tool-use call in the intelligence service, so no prior typed precedent existed. - ruff: import sort, drop pointless f-string prefixes, wrap one long line. mypy --strict and ruff both clean; full suite still 105/105 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: retrigger CI (no functional change) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…igests (Suncrest-Labs#898) Adds the Go-side data plumbing for Suncrest-Labs#859's periodic digest: a digest_cadence notification preference (off/weekly/monthly, opt-out respected), a user_digests cache/audit table for one-generation-per-period, a digest-ledger source endpoint exposing deterministic period deposit/yield/ streak facts for the intelligence service to narrate via the relay, and a leader-elected daily scheduler job that generates and delivers digests through the existing notification dispatcher. This PR covers the Go backend groundwork only. The intelligence-service narrative generation (grounded LLM prompt, zero-save honesty handling, attention items, Redis caching), frontend insights card, and test coverage described in Suncrest-Labs#859's acceptance criteria are not yet implemented — tracked as follow-up. Suncrest-Labs#865, Suncrest-Labs#864, and Suncrest-Labs#856 are referenced per this repo's issue numbering but have no implementation in this branch. Note: this environment has no Go toolchain available, so these changes are reviewed manually but not compiled or test-run locally.
- Remove unused startKYC variable from rotate_keys/main.go (go vet error). - Remove unused time import from backfill_kyc_encryption/main.go (go vet error).
…-position support Add rich progress visualization for savings goals and vaults: - Segmented progress bar distinguishing locked vs flexible portions - Principal vs earned yield composition breakdown - Maturity timeline for locked positions with boost badges and unlock dates - Probabilistic projection band (confidence interval + success probability) - Constructive at-risk messaging when goal is off track - Multi-asset vault composition donut (reuses existing recharts pattern) - Celebration/completion state, encouraging empty state - Respects reduced-motion preferences via existing useReducedMotion hook - All states: empty, in-progress-with-locks, at-risk, completed - Backward compatible: falls back to simple progress bar when rich data absent Closes Suncrest-Labs#869
…s guard, AI opt-out - Suncrest-Labs#928: apps/intelligence/app/services/prometheus.py's stream_chat caught every Claude error identically (generic "trouble connecting" message). Added a specific anthropic.APIStatusError handler that distinguishes 429 (rate-limited) / 529 (overloaded) with a clearer "receiving a lot of requests, try again shortly" message, while other API status errors and non-API exceptions keep the existing generic fallback. Neither chat.py nor ws_chat.py ever surfaced a raw 500 for this — stream_chat already caught everything — but the message didn't call out the specific, actionable rate-limit/overload case. - Suncrest-Labs#930: added tests/test_retrieval_relevance.py. retrieval.py's "relevance filtering" is intent-based section gating (route_query -> which of GOALS/TRANSACTIONS/YIELD_LANDSCAPE/POSITIONS get fetched), not numeric relevance scoring — tests confirm sections NOT matched by the query's intents are never even fetched (not just absent from output), plus the existing empty-result fallback behavior. tests/test_retrieval.py (from Suncrest-Labs#852) already covered routing and basic empty-fallback; this fills the specific "sections excluded, not just empty" gap Suncrest-Labs#930 asks for. - Suncrest-Labs#931: apps/intelligence/app/services/defillama.py already had a TTL cache; added the staleness guard the issue is actually about — every successful fetch also writes a long-TTL (24h) "last known good" copy, and a live-fetch failure after the short-TTL entry has expired now serves that stale copy instead of an empty list, so a DefiLlama outage degrades to slightly-stale yield data instead of no data. - Suncrest-Labs#935: apps/api/internal/service/goal_coaching_scheduler.go's weekly AI goal-coaching job iterated every active goal and called the intelligence service unconditionally — no opt-out check at all, unlike nudge_engine_service.go's existing NudgesEnabled gate for generic nudges. Added the same nudge.PreferenceChecker gate before any intelligence-service call, and added an explicit ai_insights_enabled field to CoachingRequest (both Go and the intelligence service's Pydantic model) so the intelligence service also refuses to generate content when told a user opted out — enforcement independent of the caller, not just "trust the API already checked." Defaults to enabled so on-demand (user-initiated) coaching requests, which opt-out doesn't apply to, are unaffected. Verification notes: - Python (apps/intelligence): full suite run locally in a fresh uv venv — 212 passed, including all new/changed tests. - Go (apps/api): no Go toolchain was available in the environment this was authored in, so main.go / goal_coaching_scheduler.go / model.go and their test changes could not be compiled or run locally — reviewed by hand for signature/interface consistency (nudgeHistoryRepo already implements nudge.PreferenceChecker; the *T-vs-T receiver on the new recordingGoalCoachingClient test double satisfies the GoalCoachingClient interface). Please confirm via CI or a local `go build ./... && go test ./...` before merging. Closes Suncrest-Labs#928 Closes Suncrest-Labs#930 Closes Suncrest-Labs#931 Closes Suncrest-Labs#935
…e-while-revalidate (Suncrest-Labs#827) Adds a generic Redis-backed cache (internal/cache) sitting in front of any compute function: in-process single-flight collapses concurrent same-key misses to one compute regardless of Redis, a best-effort cross-process Redis lock reduces duplicate work across instances, TTLs are jittered to avoid synchronized expiry, and soft/hard TTLs enable serve-stale-while-revalidate (a stale value is returned immediately while a background refresh runs). Namespace-scoped Invalidate targets a single key; a nil Redis client degrades the cache to in-process-only behavior rather than failing. closes Suncrest-Labs#827
…an-out (Suncrest-Labs#828) Extends the WebSocket hub so events reach connected clients regardless of which API instance holds their socket or produced the event: each instance publishes broadcast events to Redis pub/sub and re-injects events received from other instances into its own local delivery path, skipping its own echoed publishes via an origin-instance tag. Per-topic Redis subscriptions are reference-counted against local subscriber counts so an instance only subscribes to channels its own clients actually need, and are released on the last local unsubscribe or on graceful shutdown. Adds Redis-backed presence tracking with a heartbeat-refreshed TTL so a crashed instance's presence entries self-expire rather than lingering. Adds per-IP connection limits (429 on exceeding the configured cap) and keeps the existing slow-client backpressure (disconnect on a full send buffer) intact. All of this degrades to the pre-existing single-instance in-process-only behavior when no Redis client is configured (nil-safe throughout, matching the codebase's existing dual-mode convention for middleware.NewLimiter). Tests include a real two-Redis-sharing two-Hub cross-instance delivery test, an own-event-not-double-delivered test, a reconnect-moves-subscriptions test, a cross-instance presence test, a slow-client disconnect test, and a per-IP limit rejection test — all passing against a real Redis instance. closes Suncrest-Labs#828
…ences, dedup and delivery tracking (Suncrest-Labs#829) Adds a suppressibility Category (safety/transactional/promotional) per EventType: safety notifications always bypass preference and rate-limit checks (a breaker trip must never be silently opted out of), promotional fully honors opt-out, transactional sits in between. Preferences can now be resolved per-category via an optional CategoryPreferenceStore seam (a Postgres-backed GetForCategory/SetCategoryOverride is added to NotificationRepository, storing overrides in a new category_overrides JSONB column added by migration 069) while stores that only implement the existing flat PreferenceStore keep working unchanged. Adds dedup (in-memory and Redis-backed, SET-NX-EX) and per-user-per-category rate limiting (reusing middleware.NewLimiter's existing dual-mode Redis/ in-process pattern) — both suppress a Send while still persisting the notification with a recorded SuppressedReason, so a suppressed message is auditable rather than silently dropped. A suppressed or delivered notification's outcome is tracked per channel (Delivered/Error/IsFallback) via a new optional DeliveryOutcomeRecorder seam, with a Push/Email failure falling back to WebSocket delivery (deduped against a WebSocket delivery already in that event's normal channel matrix). Failed Email/Push deliveries enqueue a durable retry job through the existing job queue (jobqueue.Client); the job handler redelivers via the specific channel that failed. Dispatcher.Stats() exposes per-category attempted/delivered/failed/ suppressed counts for a metrics endpoint. Also fixes the stale "TODO: Fix interface implementation" in main.go that had left NewWebSocketChannel commented out — WebSocketHub's PushToUser signature already matched Hub's once Suncrest-Labs#828's hub.go changes landed, so in-app websocket delivery through the notification dispatcher is now actually wired, not just persisted-and-discarded. Deliberately deferred (disclosed rather than silently skipped): HTTP handler/frontend surface for editing category preferences (the existing flat-preference handler/settings page is unchanged); migrating goal_milestone_notifier's own notified_milestones dedup onto the new generic Deduplicator (that table is a permanent, non-windowed, correctness-sensitive dedup — migrating it is exactly the kind of unreviewed, regression-risk change this PR intentionally avoids); real SMTP/ push provider integrations (the existing MailSender/PushSender seams and their Noop/Recording implementations are unchanged). Also fixes CI: the api job's Redis service only exported REDIS_URL, but every Redis-backed test (existing internal/cache tests included) skips via REDIS_ADDR per the established convention, so these tests have been silently skipping in CI. Sets REDIS_ADDR alongside REDIS_URL. closes Suncrest-Labs#829
…actually run The api job's redis service was only exposed to tests via REDIS_URL, but every Redis-backed test in this codebase (internal/cache, internal/ middleware's rate limiter, and this PR's internal/ws and internal/ notifications tests) skips via t.Skip when REDIS_ADDR specifically is unset. That means these tests have been silently skipping in CI even though a real Redis service was running right next to them the whole time.
…ailover (Suncrest-Labs#830) Adds Aggregate: queries every healthy registered source for a data type in parallel (each bounded by a per-source timeout so one slow source cannot stall the result), then reconciles responses via median-with-deviation-band outlier rejection — a source more than MaxDeviationBPS from the pre-filter median is discarded before the final median is recomputed from survivors, so a single bad or manipulated print cannot move the consensus. Returns Unavailable only when zero sources respond; a lone responding source below MinAgreeingSources still produces a value (preserving the existing priority-failover availability guarantee) but with reduced Confidence rather than a blind pass-through, so a caller that needs full consensus can gate on Confidence instead of merely on "a value came back". Adds HealthTracker: per-source consecutive-failure count, last error, and an exponential backoff window (5s base, doubling per consecutive failure, capped at 5m) during which a source is skipped rather than queried on every request; a success clears the failure history immediately. Wires this into RateService.fetchXLM (internal/oracle/service.go), replacing the previous "try providers in priority order, first success wins" loop with real two-source (Horizon, DeFiLlama) consensus — the existing XLM sanity-bounds check is kept as a second, independent defense against every source being corrupted in the same direction, which deviation-band rejection alone can't catch. ExchangeRate gains Confidence and SourcesUsed fields (empty/zero for rates that don't go through the aggregator, e.g. the fixed USDC/USD peg) and a MeetsConfidenceThreshold helper for downstream consumers to gate on. All existing service_test.go cases pass unchanged, including the priority-style single-surviving-source assertions (SourceName() reports a lone source's own name, matching the prior Source field behavior exactly, and only joins names when more than one source genuinely agreed). Deliberately deferred (disclosed rather than silently skipped): a second independent source for TVL and for the DeFiLlama-sourced portions of the APY pipeline (apy_service.go / apy_refresh.go) — those currently have only one real external provider each in this codebase, and standing up a second genuine external data provider integration is out of scope here; migrating risk_service.go and the on-chain attestation signer (a separate contracts repo) onto Confidence gating. One added cost worth flagging: because Aggregate queries every healthy source in parallel rather than stopping at the first success, DeFiLlama is now called on every XLM/USD refresh even when Horizon succeeds, not only as a fallback. Tests cover: agreeing sources produce the correct median consensus; a wildly-off outlier is rejected without moving the consensus; all-but-one source down yields a value with reduced (not zero, not full) confidence rather than Unavailable; every source down is Unavailable; a slow source times out without stalling the result; a source is skipped once unhealthy and re-probed after its backoff window elapses; backoff grows with consecutive failures; a success clears failure history; confidence decay for staleness reaches exactly half at maxAge. closes Suncrest-Labs#830
apysnapshot/model.go stores APY snapshots straight from the DeFiLlama poller with no sanity check, so a bad upstream reading (oracle glitch, scraping error, a genuinely manipulated pool) flows straight into vault APY history and user-facing yield figures. This adds a guard that flags implausible jumps before a snapshot is persisted, complementing the oracle aggregation/failover work in Suncrest-Labs#830. - add apysnapshot.DetectAnomalousJump: compares a new snapshot's APY to the protocol's most recent prior reading and flags moves of more than AnomalyJumpMultiplier (3x) in either direction, skipping near-zero baselines (< 0.5%) where large percentage swings are normal noise - add Flagged/FlagReason fields to APYSnapshot, persisted via migration 081 (apy_snapshots.flagged, flag_reason) - wire the guard into APYService.poll via a new flagIfAnomalous step that looks up the most recent snapshot within a 48h lookback window and flags (does not reject) the incoming snapshot, so a genuine market dislocation doesn't starve history/oracle failover of data
- api: add optional min/max per-contribution limits on savings goals, validated at deposit time in DepositSplit (savingsgoal/model.go) - api: let admins publish/edit/remove curated savings goal templates via domain/admin, growing the catalog beyond the pre-built Suncrest-Labs#778 defaults without a redeploy - dapp: add CSV/PDF export to the savings calculator, reusing the existing lib/export utilities Closes Suncrest-Labs#918 Closes Suncrest-Labs#919 Closes Suncrest-Labs#922
…Labs#1000) * fix(api): restore soft-delete code erased by merge 9c30de1 (Suncrest-Labs#994) Merge 9c30de1 (via PR Suncrest-Labs#985) resolved conflicts in the savings goal domain model and Postgres repository by taking the contributor's stale branch side wholesale, discarding the soft-delete work that had already landed in dev via Suncrest-Labs#984 (Suncrest-Labs#924). Migration 081 survived because it had no conflict, so the database grew a deleted_at column while the Go code that reads it was deleted, leaving dev unable to compile. Both conflicting hunks were additive on each side, so the correct resolution keeps both: - Restore DeletedAt on SavingsGoal alongside MinContribution and MaxContribution from Suncrest-Labs#922. - Restore ErrGoalNotDeleted and ErrRecoveryWindowExpired alongside ErrInvalidContributionLimits and ErrContributionOutOfRange. - Restore GetByIDIncludingDeleted, whose interface method was still declared but whose implementation was dropped. - Restore the deleted_at IS NULL guards on GetByID, GetByShareToken and Update, and extend every SELECT feeding scanSavingsGoalWithShare to the same column list. ListDeletedOlderThan also selected 22 columns while sharing the 24-column scanner. That mismatch predates this fix but sits on the purge path, so it is corrected here rather than left as a latent runtime failure. Verified: go build ./..., go vet ./... and go test -count=1 -short ./... all clean across apps/api. * fix(db): renumber colliding migrations to unique prefixes (Suncrest-Labs#995) Eleven migration pairs were fighting over five version numbers (060, 061, 069, 070, 081). Several PRs developed in parallel each claimed what was "the next" number when work started rather than when the PR opened. With duplicate versions golang-migrate's ordering is undefined, so which migration applies — or whether one is skipped — depends on filesystem iteration order. This also failed the "Check migration collisions" CI gate, which runs before Build and so blocked the whole API job. At each colliding prefix the earliest-landed migration keeps its number and later arrivals move to 084+ in the order they landed: 060_create_timeseries_store -> 084 061_create_reconciliation_audit -> 085 069_add_notification_category_preferences -> 086 069_apy_snapshots_unique_protocol_time -> 087 070_redesign_sessions_for_rotation -> 088 081_add_apy_snapshot_anomaly_flag -> 089 Ordering constraints are preserved: - 069_encrypt_kyc_documents and 070_encrypt_kyc_documents_index keep their numbers and stay adjacent. The index is built CONCURRENTLY against a column the 069 migration adds, so it must continue to run after it. - 087 and 089 both target apy_snapshots. They are independent (a unique constraint vs. two new columns), and 087 < 089 preserves the original 069 < 081 relative order. - Every base table the moved migrations touch is created earlier (013 sessions, 029 notification_preferences, 044 apy_snapshots), so all of them remain safely above their dependencies. - 081_add_savings_goal_soft_delete keeps its number; it backs the code restored in the previous commit. File contents are unchanged — these are pure renames of both the .up.sql and .down.sql halves. No code, docs, or CI config referenced the old filenames. The README's own conflict check had the same blind spot that let these collisions through: it deduplicated .up and .down as separate entries, so it never reported a duplicate. Replaced with the exact check CI runs, plus guidance to claim a number at PR-open time and a note that renumbering after a version is recorded in schema_migrations is not always safe. Verified: the CI collision check now passes, all 88 up/down pairs match, and go build ./... plus go test -count=1 -short ./... are clean.
…age 1/N) (Suncrest-Labs#877) Relates to Suncrest-Labs#809 — Stage 1 of a staged implementation (conversion & limits only). Token hardening, transfer-sync/reentrancy, lock semantics, backend indexing, and documentation are separate follow-up stages, not included here. ## Problem No standard tokenised-vault interface exists on Nester's vault. This stage adds the conversion/limit surface (convert_to_shares, convert_to_assets, total_assets, max_deposit, max_withdraw, max_redeem) with explicit, tested rounding rules — the foundation the later stages build on. ## Changes - New packages/contracts/contracts/vault/src/conversion.rs — pure, Soroban-environment-free arithmetic module: - mul_div_down / mul_div_up using checked quotient/remainder decomposition (avoids the unchecked x*y overflow present in the existing shares_for_deposit_math/amount_for_shares_math). - assets_to_shares_down/up, shares_to_assets_down/up. - Explicitly distinguishes bootstrap (total_shares == 0 → 1:1) from insolvency (total_assets == 0 with total_shares > 0 → returns ContractError::InvalidOperation). This fixes an existing bug where an insolvent-but-live vault would silently mint shares 1:1 — a known share-inflation exploit vector. - vault/src/lib.rs — new TokenisedVault entrypoints. max_deposit/ max_withdraw/max_redeem return zero (never revert) when the action is impossible, per the interface contract integrators depend on. - libs/common/src/lib.rs — shared TokenisedVault trait definition. - tests/integration — added non-zero-remainder rounding tests in both directions. Also fixed: share_price_tests.rs existed but was never declared in integration/mod.rs, so it had never actually been compiled or run before this change. ## Rounding rule (documented in conversion.rs) - Value a user receives rounds down. - Value a user must pay rounds up. - Every remainder favors the vault, never the user. ## Verification - cargo check -p vault-contract -j 1: passed. - Test execution blocked locally by a Windows GNU linker limitation (export ordinal too large: 66278) — unrelated to code correctness. Full test run needs to happen via CI or a non-Windows-GNU environment before merge confidence. - git diff --check: clean. No unrelated formatter/CRLF churn included. - Scope confirmed: only conversion/limit logic touched. No token, transfer-sync, lock, backend, or documentation changes in this stage. ## Open items for maintainer / next stage Two pre-existing security issues were found during investigation, unrelated to Suncrest-Labs#809's literal scope: 1. vault_token's burn_from is not vault-restricted — any address with sufficient allowance can burn, not just the vault. 2. Negative amounts are not consistently rejected across transfer/burn paths, which can invert balance movements. Recommend deciding whether these are fixed as part of a later stage of this PR series or filed as separate security issues — flagging now so they aren't lost. ## Files changed - packages/contracts/contracts/vault/src/conversion.rs (new) - packages/contracts/contracts/vault/src/lib.rs - packages/contracts/libs/common/src/lib.rs - packages/contracts/tests/integration/src/integration/mod.rs - packages/contracts/tests/integration/src/integration/share_price_tests.rs Co-authored-by: Cursor <cursoragent@cursor.com>
… (Auto-Generated) (Suncrest-Labs#983) * Fix issue Suncrest-Labs#947: update apps/api/internal/domain/vault/position.go * Fix issue Suncrest-Labs#947: update apps/api/internal/domain/vault/position_test.go
…Suncrest-Labs#986) The users INSERT in scripts/seed.sql already matched the post-007 schema (wallet_address/display_name/kyc_status, no email/name) and already granted the test user an admin user_roles row — verified identical against upstream Suncrest-Labs/nester dev. What was actually broken: nothing in docker-compose.yml or the Makefile ever executed seed.sql, so `make dev` never seeded the database despite the README claiming it does. Added a one-shot `seed` compose service that runs seed.sql via psql once the api service (and its auto-migrations) are healthy. Since seed now runs on every `make dev`, also added ON CONFLICT (id) DO NOTHING to the vaults/allocations/settlements INSERTs (users/user_roles already had it) so repeated runs against a persisted volume don't fail on duplicate keys. Verified by applying all 79 migrations to a fresh Postgres volume and running seed.sql twice in a row — both succeed, second run is a no-op. Closes Suncrest-Labs#608
…ss-contract pause validation (Suncrest-Labs#1004)
- Fix import order in claude.py - Fix indentation in ws_chat.py - Fix line length errors in test files - Restore prometheus.py from dev - All ruff checks passing
- Add return type annotations to get_client and get_model_id - Add type ignore for pytz import - Remove unused ResponsePreferences import - Fix import order in test_defillama_staleness.py
cyb3ralee
force-pushed
the
feat/natural-language-goal-creation
branch
from
July 30, 2026 14:53
f7d498a to
2bf94bc
Compare
Contributor
|
This has the largest conflict surface of any open PR right now — 21 conflicts against
The savings-goal area has been under heavy concurrent change ( At 21 conflicts I'd genuinely suggest considering a fresh branch off current One thing worth knowing before you rerun CI: I'll do the full review once it's mergeable. |
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
20 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
Adds natural-language goal creation using Claude structured output. Users can create savings goals by describing them in plain language.
Changes
goal_extractor.pyservice with Claude structured outputnatural_language_goal.pyrouter for API endpointsAPI Endpoints
POST /intelligence/extract-goal- Extract structured goal from natural languagePOST /intelligence/confirm-goal- Confirm and create the extracted goalCloses #853
Summary by CodeRabbit
X-Request-Idsupport across intelligence endpoints.