Repository navigation
ROB-2034: Limit evidence size - #978
Conversation
WalkthroughIntroduces evidence truncation utilities and integrates them into Supabase DAL paths while filtering out specific enrichment types. Adds a new env var for truncation limits. Updates tests for truncation behavior. Adjusts pre-commit mypy invocation to project-wide and excludes dist/ from mypy in pyproject. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DAL as SupabaseDAL
participant DB as Supabase
participant TR as TruncationUtils
Caller->>DAL: get_issue_data / get_workload_issues / get_configuration_changes
activate DAL
DAL->>DB: Query evidence/changes\nwith NOT IN enrichment_type blacklist
DB-->>DAL: Rows (evidence/changes)
DAL->>DAL: Extract relevant items
DAL->>TR: truncate_evidences_entities_if_necessary(list)
TR-->>DAL: Truncated list (in-place)
DAL-->>Caller: Result with filtered + truncated evidence
deactivate DAL
note over DAL,TR: Truncation limit from MAX_EVIDENCE_DATA_CHARACTERS_BEFORE_TRUNCATION
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/supabase_dal.py (1)
296-324: Add type hints tounzip_evidence_fileper repo policy.Keeps behavior, satisfies mypy.
- def unzip_evidence_file(self, data): + from typing import Any, Dict + def unzip_evidence_file(self, data: Dict[str, Any]) -> Dict[str, Any]:
🧹 Nitpick comments (1)
holmes/core/supabase_dal.py (1)
325-341: Makeextract_relevant_issuesconfigurable and add type hints.Given the RCA path now excludes
text_fileat SQL, the unzip branch is dead here. Suggest parameterizing to avoid confusion and align with typing guidelines.Apply:
- def extract_relevant_issues(self, evidence): + from typing import Any, Dict, List + def extract_relevant_issues(self, evidence: Any, include_text_files: bool = True) -> List[Dict]: enrichment_blacklist = {"text_file", "graph", "ai_analysis", "holmes"} - data = [ + data: List[Dict] = [ enrich for enrich in evidence.data if enrich.get("enrichment_type") not in enrichment_blacklist ] - - unzipped_files = [ - self.unzip_evidence_file(enrich) - for enrich in evidence.data - if enrich.get("enrichment_type") == "text_file" - ] - - data.extend(unzipped_files) + if include_text_files: + unzipped_files = [ + self.unzip_evidence_file(enrich) + for enrich in evidence.data + if enrich.get("enrichment_type") == "text_file" + ] + data.extend(unzipped_files) return dataAnd in this flow:
- data = self.extract_relevant_issues(evidence) + data = self.extract_relevant_issues(evidence, include_text_files=False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/supabase_dal.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/core/supabase_dal.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/core/supabase_dal.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (1)
holmes/core/supabase_dal.py (1)
371-376: Exclude text_file evidence at the DB layer — LGTM; confirm RCA-only impact.Filtering out
enrichment_type == "text_file"server‑side matches ROB-2024 and reduces payload. Note this makes the unzip path inextract_relevant_issuesunreachable for this flow. Please confirm that RCA is intended to ignore text files entirely while other flows (e.g.,get_workload_issues) keep current behavior.Optional: for consistency and less transfer, consider also pushing the rest of the blacklist to SQL and narrowing the projection:
- evidence = ( - self.client.table(EVIDENCE_TABLE) - .select("*") - .filter("issue_id", "eq", issue_id) - .filter("enrichment_type", "neq", "text_file") - .execute() - ) + evidence = ( + self.client.table(EVIDENCE_TABLE) + .select("id,data,enrichment_type") + .filter("issue_id", "eq", issue_id) + .not_.in_("enrichment_type", ["text_file", "graph", "ai_analysis", "holmes"]) + .execute() + )
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
holmes/core/supabase_dal.py (3)
265-266: Server-side exclude looks right; log the “no data” case and confirm scope (RCA vs config changes).Filtering out text_file at the query is good. However, this also affects get_configuration_changes results. Is excluding text_file here intentional for configuration-change history as well (PR title mentions RCA)? Add a “no data” log that reflects the new filter for easier triage.
Apply this small logging tweak:
- if not len(change_data_response.data): - return None + if not change_data_response.data: + logging.info( + "No change content in %s for issues=%s after excluding enrichment_type='text_file'.", + EVIDENCE_TABLE, changes_ids + ) + return None
374-376: Good swap to.eq; add empty-evidence debug and confirm uniqueness constraints.The move to
.eq("issue_id", issue_id)is cleaner. Given we now exclude text_file, evidence may be empty; add a debug log. Also, do we rely on issue_id being globally unique, or should we defensively includeaccount_idin the evidence query?evidence = ( self.client.table(EVIDENCE_TABLE) .select("*") .eq("issue_id", issue_id) .neq("enrichment_type", "text_file") .execute() ) + if not evidence.data: + logging.debug( + "No evidence for issue_id=%s after excluding enrichment_type='text_file'.", + issue_id + )
523-524: Exclude is fine; fix return type and consider aligning server-side filter with in-function blacklist.
- OK to exclude text_file at source. Note: get_workload_issues is annotated to return List[str] but returns structured evidence objects via extract_relevant_issues. Update the signature to avoid mypy issues.
- Optional: to reduce payload, mirror extract_relevant_issues’ blacklist on the server using
not_.in_(only if product intent matches).Optional tighter filter:
- .neq("enrichment_type", "text_file") + .not_.in_("enrichment_type", ["text_file"])Update the function signature (outside this hunk):
def get_workload_issues(self, resource: dict, since_hours: float) -> List[Dict]: ...Add a debug for empty results:
res = ( self.client.table(EVIDENCE_TABLE) .select("data, enrichment_type") .in_("issue_id", unique_issues) .neq("enrichment_type", "text_file") .execute() ) + if not res.data: + logging.debug( + "No workload evidence for %s after excluding enrichment_type='text_file'.", + svc_key + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/supabase_dal.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/core/supabase_dal.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/core/supabase_dal.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/core/supabase_dal.py (2)
489-534: Return type no longer matches implementation.
get_workload_issuesnow returns a list of evidence dicts, notList[str]. Update the annotation (and importAny).-from typing import Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple-def get_workload_issues(self, resource: dict, since_hours: float) -> List[str]: +def get_workload_issues(self, resource: dict, since_hours: float) -> List[Dict[str, Any]]:Also applies to: 8-8
331-344: Fix unreachable 'text_file' unzip — DB blacklist strips entries before unzip.ENRICHMENT_BLACKLIST contains "text_file" (holmes/core/supabase_dal.py:50) and queries use .not_.in_(ENRICHMENT_BLACKLIST) (lines ~267 / 378 / 527), so the unzip comprehension in get_issue_data (holmes/core/supabase_dal.py:331–344) will never see "text_file" entries.
- Fix: either remove "text_file" from the DB-level blacklist or fetch evidence without that filter in get_issue_data and apply the blacklist only where intended (add a flag/context to toggle the DB filter).
Locations: holmes/core/supabase_dal.py:50, 267, 331–344, 378, 527.
🧹 Nitpick comments (4)
holmes/core/truncation/dal_truncation_utils.py (1)
7-10: Minor typing/PEP8 polish.Space after colon in annotations and a shared suffix constant improve readability and keep tests consistent.
Apply this diff:
-def truncate_string(data_str:str) -> str: +TRUNCATION_SUFFIX = "-- DATA TRUNCATED TO AVOID HITTING CONTEXT WINDOW LIMITS" + +def truncate_string(data_str: str) -> str: - if data_str and len(data_str) > MAX_EVIDENCE_DATA_CHARACTERS_BEFORE_TRUNCATION: - return data_str[:MAX_EVIDENCE_DATA_CHARACTERS_BEFORE_TRUNCATION] + "-- DATA TRUNCATED TO AVOID HITTING CONTEXT WINDOW LIMITS" + if data_str and len(data_str) > MAX_EVIDENCE_DATA_CHARACTERS_BEFORE_TRUNCATION: + return data_str[:MAX_EVIDENCE_DATA_CHARACTERS_BEFORE_TRUNCATION] + TRUNCATION_SUFFIX return data_strholmes/core/supabase_dal.py (3)
33-33: Remove unused import.
truncate_stringisn’t used.-from holmes.core.truncation.dal_truncation_utils import truncate_evidences_entities_if_necessary, truncate_string +from holmes.core.truncation.dal_truncation_utils import truncate_evidences_entities_if_necessary
369-371: Drop unused exception variable.Avoid Ruff F841; the exception is not used.
-except Exception as e: # e.g. invalid id format +except Exception: # e.g. invalid id format
531-537: Optional: restructure try/except for clarity (TRY300).Return in an
elseblock to avoid shadowing/partially-initialized vars.- try: + try: res = ( self.client.table(EVIDENCE_TABLE) .select("data, enrichment_type") .in_("issue_id", unique_issues) .not_.in_("enrichment_type", ENRICHMENT_BLACKLIST) .execute() ) - - relevant_issues = self.extract_relevant_issues(res) - truncate_evidences_entities_if_necessary(relevant_issues) - return relevant_issues + else: + relevant_issues = self.extract_relevant_issues(res) + truncate_evidences_entities_if_necessary(relevant_issues) + return relevant_issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
holmes/common/env_vars.py(1 hunks)holmes/core/models.py(1 hunks)holmes/core/supabase_dal.py(6 hunks)holmes/core/truncation/dal_truncation_utils.py(1 hunks)tests/core/truncation/test_dal_truncation_utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/core/models.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
tests/core/truncation/test_dal_truncation_utils.pyholmes/common/env_vars.pyholmes/core/truncation/dal_truncation_utils.pyholmes/core/supabase_dal.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/core/truncation/test_dal_truncation_utils.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/core/truncation/test_dal_truncation_utils.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/core/truncation/dal_truncation_utils.pyholmes/core/supabase_dal.py
🧬 Code graph analysis (2)
tests/core/truncation/test_dal_truncation_utils.py (1)
holmes/core/truncation/dal_truncation_utils.py (1)
truncate_evidences_entities_if_necessary(12-17)
holmes/core/supabase_dal.py (1)
holmes/core/truncation/dal_truncation_utils.py (2)
truncate_evidences_entities_if_necessary(12-17)truncate_string(7-10)
🪛 Ruff (0.12.2)
holmes/core/supabase_dal.py
369-369: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
533-533: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
holmes/common/env_vars.py (1)
85-87: LGTM.New env var is consistent with existing patterns. No issues.
tests/core/truncation/test_dal_truncation_utils.py (1)
118-146: Tests look right and cover key edge cases.These will pass once None/missing-data handling in the truncation util is fixed.
After applying the util fix, please run the truncation test module to confirm all cases pass.
Also applies to: 149-169, 170-195, 196-224
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/core/supabase_dal.py (2)
333-348: Use the set for membership and add type hints to satisfy mypy.
- Filter should check against ENRICHMENT_BLACKLIST_SET (O(1) membership).
- This method lacks type hints; add a lightweight Protocol for
evidence.data.Apply this diff in this block:
- def extract_relevant_issues(self, evidence): + def extract_relevant_issues(self, evidence: "HasData") -> list[dict]: @@ - if enrich.get("enrichment_type") not in ENRICHMENT_BLACKLIST + if enrich.get("enrichment_type") not in ENRICHMENT_BLACKLIST_SETAnd add this helper near the imports (outside this range):
from typing import Any, Protocol class HasData(Protocol): data: list[dict[str, Any]]
492-495: Action required: fix get_workload_issues return-type mismatch (annotated List[str] but returns dicts).
- holmes/core/supabase_dal.py:492 — either change the signature to -> list[dict] if enriched evidence dicts are intended, or change the implementation to return List[str] (IDs/messages) to match current annotation.
- server.py:194–199 — workload_alerts is typed as list[str] and consumes dal.get_workload_issues(...); update server logic/types if you switch to list[dict].
- tests/llm/utils/mock_dal.py:72 — mock returns [] and won’t catch this mismatch.
♻️ Duplicate comments (3)
holmes/core/supabase_dal.py (3)
380-382: Same concern:.not_.in_must get a list, not a set.Safe once ENRICHMENT_BLACKLIST is a list per earlier fix.
530-531: Same concern:.not_.in_must receive a list.This will be correct once ENRICHMENT_BLACKLIST is changed to a list.
52-53: Bug: using a set with PostgREST.in_/.not_.in_can break serialization; keep a list for queries and a set for Python membership.PostgREST expects a list/array. Passing a set risks runtime errors or incorrect SQL. Also add explicit types per mypy.
Apply this diff to define both list and set:
-ENRICHMENT_BLACKLIST = {"text_file", "graph", "ai_analysis", "holmes"} +ENRICHMENT_BLACKLIST: list[str] = ["text_file", "graph", "ai_analysis", "holmes"] +ENRICHMENT_BLACKLIST_SET: set[str] = set(ENRICHMENT_BLACKLIST)
🧹 Nitpick comments (1)
holmes/core/supabase_dal.py (1)
534-537: Ruff TRY300: move return into anelseafter thetry/except.Minor style/readability tweak; avoids returns inside
try.- relevant_issues = self.extract_relevant_issues(res) - truncate_evidences_entities_if_necessary(relevant_issues) - return relevant_issues + relevant_issues = self.extract_relevant_issues(res) + truncate_evidences_entities_if_necessary(relevant_issues) + except Exception: + logging.exception("failed to fetch workload issues data", exc_info=True) + return [] + else: + return relevant_issues - except Exception: - logging.exception("failed to fetch workload issues data", exc_info=True) - return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/supabase_dal.py(6 hunks)holmes/core/truncation/dal_truncation_utils.py(1 hunks)tests/core/truncation/test_dal_truncation_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/core/truncation/dal_truncation_utils.py
- tests/core/truncation/test_dal_truncation_utils.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/core/supabase_dal.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/core/supabase_dal.py
🧬 Code graph analysis (1)
holmes/core/supabase_dal.py (1)
holmes/core/truncation/dal_truncation_utils.py (1)
truncate_evidences_entities_if_necessary(13-23)
🪛 Ruff (0.12.2)
holmes/core/supabase_dal.py
536-536: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
🔇 Additional comments (3)
holmes/core/supabase_dal.py (3)
33-35: Good: truncation utilities imported at module top.Import placement follows our Python import guideline and the helper is used below.
276-277: LGTM: truncation at the DAL boundary.In-place truncation on the retrieved rows is appropriate and keeps downstream code simple.
384-388: LGTM: evidence extraction + truncation + assignment.Flow is correct and consistent with the new helpers.
nherment
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holmes/core/supabase_dal.py (2)
52-54: Good fix: list for PostgREST + set for membership.This resolves the earlier concern about passing a set to
.in_/.not_.in_.
271-272: DB-side filter now passes a list — OK.
.not_.in_("enrichment_type", ENRICHMENT_BLACKLIST)is now safe given the list type.
🧹 Nitpick comments (4)
holmes/core/supabase_dal.py (4)
52-54: Add explicit types for mypy.Annotate the blacklist constants.
-ENRICHMENT_BLACKLIST = ["text_file", "graph", "ai_analysis", "holmes"] -ENRICHMENT_BLACKLIST_SET = set(ENRICHMENT_BLACKLIST) +ENRICHMENT_BLACKLIST: list[str] = ["text_file", "graph", "ai_analysis", "holmes"] +ENRICHMENT_BLACKLIST_SET: set[str] = set(ENRICHMENT_BLACKLIST)
381-383: Narrow selected columns to reduce payload.You only need
dataandenrichment_typefor downstream processing; avoidselect("*").- evidence = ( - self.client.table(EVIDENCE_TABLE) - .select("*") + evidence = ( + self.client.table(EVIDENCE_TABLE) + .select("data, enrichment_type") .eq("issue_id", issue_id) .not_.in_("enrichment_type", ENRICHMENT_BLACKLIST) .execute() )
385-389: DRY the “extract + truncate” pattern.This sequence appears in multiple methods; consider a tiny helper to apply both steps consistently.
535-537: Ruff TRY300: optional style tweak.Consider moving the
return relevant_issuesunder anelse:after thetryblock or after the earlyreturn []to satisfy TRY300. Purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/supabase_dal.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/core/supabase_dal.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/core/supabase_dal.py
🪛 Ruff (0.12.2)
holmes/core/supabase_dal.py
537-537: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/core/supabase_dal.py (4)
33-35: Import placement LGTM.Importing the truncation utility at module top complies with our import rule.
277-277: Confirm truncation is idempotent and shape-safe.
truncate_evidences_entities_if_necessary(change_data_response.data)mutates in place; ensure it tolerates repeated calls and the Evidence row shape used here.
338-338: Set-based membership filter LGTM.Efficient and consistent with the shared blacklist.
531-532: DB-side blacklist filter LGTM.Consistent with other call sites; reduces transfer size before truncation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyproject.toml (1)
86-86: Anchor the new mypy exclude to the repo root.Using an anchored regex avoids unintentionally excluding nested paths containing “dist”. Suggest:
- "dist/.*", + "^dist/.*",.pre-commit-config.yaml (1)
29-33: mypy will run on every commit, even for non‑Python changes.With
pass_filenames: falseand notypes/filesfilter, the hook triggers for any change. Keep repo‑wide checking but limit runs to Python diffs.- entry: poetry run mypy . - args: ["--config-file=pyproject.toml"] + entry: poetry run mypy . + args: ["--config-file=pyproject.toml", "--show-error-codes"] language: system + types: [python] pass_filenames: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
2 participants
No description provided.