Skip to content

Conversation

@arikalon1
Copy link
Contributor

No description provided.

@arikalon1 arikalon1 requested a review from RoiGlinik October 28, 2025 07:56
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

The PR increases CloudTrail query limits (--max-items from 50 to 100), adds container insights log gathering capability, introduces comprehensive memory optimization guidelines for AWS operations, and reorders imports in main.py and server.py to occur after security/certificate initialization blocks.

Changes

Cohort / File(s) Summary
AWS Template Configuration
helm/holmes/templates/mcp-servers/aws/_helpers.tpl
Increased CloudTrail query limits (--max-items: 50→100), expanded RevokeSecurityGroupIngress search limits (10→20), added container insights log filtering snippet, introduced comprehensive memory optimization guidelines with safe/max limits for CloudWatch Logs/CloudTrail/EC2/RDS/ELB/S3/Cost & Usage, added pagination best practices section
Python Import Reordering
holmes/main.py, server.py
Relocated imports (sys, USER_COLOR, json, typing, litellm, sentry_sdk, get_version, is_official_release) to occur after security/certificate initialization blocks, preserving runtime behavior while ensuring proper initialization order

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring attention:
    • Verify CloudTrail limit increases (50→100, 10→20) align with intended memory/performance constraints
    • Review new container insights bash snippet for correctness and AWS CLI syntax
    • Confirm import reordering in holmes/main.py and server.py doesn't introduce initialization order issues or circular dependencies
    • Validate memory optimization guidelines against actual AWS API constraints and pagination behavior

Possibly related PRs

  • Add test imports #1077: Directly related—both PRs modify import ordering in holmes/main.py and server.py, moving sys/USER_COLOR and certificate-related imports to after security/configuration initialization blocks.

Suggested reviewers

  • RoiGlinik
  • nherment
  • moshemorad

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning No pull request description was provided by the author. According to the evaluation criteria, the description must be related in some way to the changeset to pass this lenient check. The complete absence of a description means there is no content to evaluate for relevance to the changeset. Without any description present, it is impossible for the PR to demonstrate that the description is related to the changes being made.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Aws mcp instructions" directly refers to the primary changes in the pull request, which center on modifications to the AWS MCP server instructions in the helm/holmes/templates/mcp-servers/aws/_helpers.tpl file. The changes include increased CloudTrail limits, new container insights shell snippets, and comprehensive memory optimization guidelines. While the PR also includes import reordering changes in holmes/main.py and server.py, these appear to be supporting modifications to the main AWS MCP instructions work. The title is concise, clear, and specifically summarizes the primary change without including unnecessary noise.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch aws-mcp-instructions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
helm/holmes/templates/mcp-servers/aws/_helpers.tpl (5)

35-40: Tighten CloudTrail example (UTC format and explicit end time)

Add Z-suffixed ISO8601 and end-time to avoid timezone ambiguity and long scans.

- - Query CloudTrail for recent API calls: `aws cloudtrail lookup-events --start-time TIME --max-items 100`
+ - Query CloudTrail for recent API calls: `aws cloudtrail lookup-events --start-time TIME_Z --end-time NOW_Z --max-items 100`
+   - Example: `aws cloudtrail lookup-events --start-time $(date -u -d '2 hours ago' +%Y-%m-%dT%H:%M:%SZ) --end-time $(date -u +%Y-%m-%dT%H:%M:%SZ) --max-items 100`

46-50: Container Insights snippet: optional region/filter to reduce scan cost

Consider guiding users to set region and a basic filter to cut noise.

 aws logs filter-log-events \
   --log-group-name /aws/containerinsights/CLUSTER_NAME/application \
   --start-time $(date -d '1 hour ago' +%s)000 \
-  --max-items 500
+  --max-items 500 \
+  # optionally set AWS region explicitly
+  # --region $AWS_REGION \
+  # optionally narrow results
+  # --filter-pattern "ERROR|Exception"

69-70: CloudTrail “last hour” example: use Z-suffix

Use explicit Z to indicate UTC.

-aws cloudtrail lookup-events --start-time $(date -u -d '1 hour ago' +%Y-%m-%dT%H:%M:%S) --max-items 100
+aws cloudtrail lookup-events --start-time $(date -u -d '1 hour ago' +%Y-%m-%dT%H:%M:%SZ) --end-time $(date -u +%Y-%m-%dT%H:%M:%SZ) --max-items 100

79-80: Add time window to targeted CloudTrail search

Prevents scanning excessive history.

-aws cloudtrail lookup-events --lookup-attributes AttributeKey=EventName,AttributeValue=RevokeSecurityGroupIngress --max-items 20
+aws cloudtrail lookup-events \
+  --lookup-attributes AttributeKey=EventName,AttributeValue=RevokeSecurityGroupIngress \
+  --start-time $(date -u -d '4 hours ago' +%Y-%m-%dT%H:%M:%SZ) \
+  --end-time $(date -u +%Y-%m-%dT%H:%M:%SZ) \
+  --max-items 20

138-141: Remove duplicate header

This repeats an earlier “Investigation Principles” header.

-## Investigation Principles
-
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7df9aed and f8e43d1.

📒 Files selected for processing (3)
  • helm/holmes/templates/mcp-servers/aws/_helpers.tpl (5 hunks)
  • holmes/main.py (1 hunks)
  • server.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods

Files:

  • server.py
  • holmes/main.py
🧬 Code graph analysis (1)
server.py (1)
holmes/version.py (2)
  • get_version (48-130)
  • is_official_release (40-44)
⏰ 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). (1)
  • GitHub Check: build
🔇 Additional comments (4)
holmes/main.py (1)

11-12: Import move under cert guard looks good

Keeps HTTPS clients from pre-initializing before custom cert install; E402 suppressed at file-level. No issues.

Please double-check no other imports were added above the guard in future patches.

server.py (2)

11-17: Good relocation of imports under cert guard

This aligns with the bootstrap pattern and prevents premature client init.


16-16: Verify consistency of import pattern with codebase conventions

The package properly exports get_version and is_official_release from holmes/__init__.py using the correct re-export syntax. However, other files in the codebase (holmes/main.py and holmes/utils/holmes_status.py) import the same functions with # type: ignore, while server.py does not. Choose one approach for consistency:

  1. Import from the defining module: from holmes.version import get_version, is_official_release
  2. Add # type: ignore to match existing codebase pattern: from holmes import get_version, is_official_release # type: ignore[attr-defined]
helm/holmes/templates/mcp-servers/aws/_helpers.tpl (1)

221-229: Review comment is incorrect; original documentation is accurate

The original statement "CloudWatch Logs: Use --max-items and --starting-token" is correct for AWS CLI usage. The AWS CLI command aws logs filter-log-events does NOT take a --next-token flag; the CLI exposes pagination via --starting-token (to resume), --page-size and --max-items.

The reviewer's suggested fix introduces non-existent CLI parameters (--limit, --next-token). While the CloudWatch Logs API FilterLogEvents uses nextToken in the request and returns nextToken in the response, these are service-level API parameters, not CLI flags. The CLI correctly abstracts these via --starting-token and --max-items.

If the intent is to document both API and CLI usage, clarify the distinction. Otherwise, the existing documentation is accurate and requires no changes.

Likely an incorrect or invalid review comment.

Comment on lines +115 to +137
## ⚠️ MEMORY OPTIMIZATION GUIDELINES ⚠️

**The AWS MCP server can experience memory pressure with very large queries. Follow these guidelines to balance data retrieval with stability:**

### Query Limits by Service Type

| Service | Safe Limit | Max Limit | Notes |
|---------|-----------|-----------|--------|
| **CloudWatch Logs** | 500 items | 1000 items | ALWAYS use time constraints (1-hour window max initially) |
| **CloudTrail** | 200 items | 500 items | Heavy JSON payloads, use 2-hour windows initially |
| **EC2 Describe** | 500 items | 1000 items | Use filters when possible |
| **RDS/ELB** | 100 items | 200 items | Usually fewer resources |
| **S3 List** | 1000 items | 5000 items | Metadata only, not object contents |
| **Cost & Usage** | 30 days | 90 days | Use DAILY granularity, not HOURLY |

### Critical Rules
1. **NEVER download S3 object contents** (can be GBs)
2. **ALWAYS use time constraints for logs** (CloudWatch, VPC Flow Logs)
3. **ALWAYS use RECENT time constraints for logs** - Even with --max-items, AWS scans ALL data in the time range!
4. **ALWAYS use small time windows. 1-2 hours. It's ok to do multiple queries.
5. **START with recommended limits**, increase if needed
6. **If query times out**, reduce time window FIRST (not just --max-items)3. **START with recommended limits**, increase if needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typos/duplication in MEMORY GUIDELINES

Clean up duplicated numbering and punctuation for clarity.

 ## ⚠️ MEMORY OPTIMIZATION GUIDELINES ⚠️
@@
-3. **ALWAYS use RECENT time constraints for logs** - Even with --max-items, AWS scans ALL data in the time range!
-4. **ALWAYS use small time windows. 1-2 hours. It's ok to do multiple queries.
-5. **START with recommended limits**, increase if needed
-6. **If query times out**, reduce time window FIRST (not just --max-items)3. **START with recommended limits**, increase if needed
+3. **ALWAYS use RECENT time constraints for logs** — even with --max-items, AWS scans ALL data in the time range.
+4. **ALWAYS use small time windows** (1–2 hours). It's OK to do multiple queries.
+5. **START with recommended limits**, increase if needed.
+6. **If a query times out**, reduce the time window first (not just --max-items).
📝 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
## ⚠️ MEMORY OPTIMIZATION GUIDELINES ⚠️
**The AWS MCP server can experience memory pressure with very large queries. Follow these guidelines to balance data retrieval with stability:**
### Query Limits by Service Type
| Service | Safe Limit | Max Limit | Notes |
|---------|-----------|-----------|--------|
| **CloudWatch Logs** | 500 items | 1000 items | ALWAYS use time constraints (1-hour window max initially) |
| **CloudTrail** | 200 items | 500 items | Heavy JSON payloads, use 2-hour windows initially |
| **EC2 Describe** | 500 items | 1000 items | Use filters when possible |
| **RDS/ELB** | 100 items | 200 items | Usually fewer resources |
| **S3 List** | 1000 items | 5000 items | Metadata only, not object contents |
| **Cost & Usage** | 30 days | 90 days | Use DAILY granularity, not HOURLY |
### Critical Rules
1. **NEVER download S3 object contents** (can be GBs)
2. **ALWAYS use time constraints for logs** (CloudWatch, VPC Flow Logs)
3. **ALWAYS use RECENT time constraints for logs** - Even with --max-items, AWS scans ALL data in the time range!
4. **ALWAYS use small time windows. 1-2 hours. It's ok to do multiple queries.
5. **START with recommended limits**, increase if needed
6. **If query times out**, reduce time window FIRST (not just --max-items)3. **START with recommended limits**, increase if needed
## ⚠️ MEMORY OPTIMIZATION GUIDELINES ⚠️
**The AWS MCP server can experience memory pressure with very large queries. Follow these guidelines to balance data retrieval with stability:**
### Query Limits by Service Type
| Service | Safe Limit | Max Limit | Notes |
|---------|-----------|-----------|--------|
| **CloudWatch Logs** | 500 items | 1000 items | ALWAYS use time constraints (1-hour window max initially) |
| **CloudTrail** | 200 items | 500 items | Heavy JSON payloads, use 2-hour windows initially |
| **EC2 Describe** | 500 items | 1000 items | Use filters when possible |
| **RDS/ELB** | 100 items | 200 items | Usually fewer resources |
| **S3 List** | 1000 items | 5000 items | Metadata only, not object contents |
| **Cost & Usage** | 30 days | 90 days | Use DAILY granularity, not HOURLY |
### Critical Rules
1. **NEVER download S3 object contents** (can be GBs)
2. **ALWAYS use time constraints for logs** (CloudWatch, VPC Flow Logs)
3. **ALWAYS use RECENT time constraints for logs** — even with --max-items, AWS scans ALL data in the time range.
4. **ALWAYS use small time windows** (1–2 hours). It's OK to do multiple queries.
5. **START with recommended limits**, increase if needed.
6. **If a query times out**, reduce the time window first (not just --max-items).
🤖 Prompt for AI Agents
In helm/holmes/templates/mcp-servers/aws/_helpers.tpl around lines 115-137, the
MEMORY OPTIMIZATION GUIDELINES contain duplicated entries, misnumbering and
punctuation errors; clean up by removing duplicated rules/lines (the repeated
"START with recommended limits" and the stray '3.'), renumber the Critical Rules
sequentially 1–6, fix punctuation (close the parenthesis on rule 4, add periods
where needed), and ensure each rule is unique, concise, and correctly ordered
with no duplicate content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants