Ec2 connection/non static ports#1
Conversation
…and AWS EC2 instance creation
iidsample
left a comment
iidsample
left a comment
There was a problem hiding this comment.
Lets find some time to chat and go over this on a call
| cpu: 1 | ||
| memory: 512 | ||
| entrypoint: agents/finance_agent.py | ||
| provider: local |
There was a problem hiding this comment.
What does provider do here ? How will the values change when it's not local ?
There was a problem hiding this comment.
In runtime_manager.py, when the instance gets created in ensure_instances(), the logic to create the instance for the agent would be different if the provider was local vs EC2.
| ec2: | ||
| region: us-east-1 | ||
| ami_id: ami-08f44e8eca9095668 | ||
| instance_type: t2.nano |
There was a problem hiding this comment.
How will we handle different types of EC2 instances
There was a problem hiding this comment.
I see, I can make the instance_type specified specific to each agent instead of just a general instance type for all agents in EC2. This doesn't handle if you want different instance types per agent/tool, but I can think about a solution for that too if that's also wanted.
I was originally planning on using AWS's Attribute-Based Instance Type Selection, where you specify some resources an instance needs in CPU and Memory and AWS automatically picks an instance to fit that. I was thinking that would allow users to just pick CPU and Mem constraints instead of a specific instance.
| @@ -0,0 +1,3 @@ | |||
| class ExampleAgent: | |||
| security_group_ids: | ||
| - sg-08d81e58ac5818c60 | ||
| ssh_user: ec2-user | ||
| ssh_private_key_path: /home/ec2-user/.ssh/saakec2.pem |
There was a problem hiding this comment.
Can we avoid having private keys
There was a problem hiding this comment.
Yup, stupid mistake. Used Private Keys as I couldn't get the auth for AWS SSM or EC2 Instance Connect, but at the very least I won't hardcode this private key path into the commit. Let me know if you want to use SSM or Instance Connect, both of which don't use private keys although Instance Connect uses temp keys.
There was a problem hiding this comment.
Why do we need a new docker file here ? We containerize everything here
There was a problem hiding this comment.
Yes, completely useless, removed both generic-agent and global-controller Dockerfiles.
| pass # Container didn't exist, that's fine | ||
|
|
||
| logger.info("Stale container cleanup complete.") | ||
| logger.info("Stale Redis container cleanup complete.") |
There was a problem hiding this comment.
Yeah, looked into this and this function's purpose changed from a full reset to just resetting shared state and data. I simply just reverted all the code to the original.
| connect_host = "localhost" if host in ("localhost", "127.0.0.1") else host | ||
| self.node_redis[host] = RedisClient( | ||
| host=connect_host, port=redis_port, | ||
| def ensure_host_redis(self, host, user=None, redis_port=6379, ssh_host=None): |
There was a problem hiding this comment.
Few things I don't like Redis container is being launched here. Why do we need to this here ?
| ) | ||
| self._on_controller_unhealthy(name, host, port) | ||
| self._last_status[(host, port)] = status | ||
| for instance in self.runtime_manager.list_instances(): |
There was a problem hiding this comment.
Let's chat about runtime manager, what are you accomplishing with it
| self._shipped_images.add((image, host)) | ||
|
|
||
| def _ship_image_registry(self, image, host, user): | ||
| def _sync_project_to_host(self, host, user, remote_dir): |
There was a problem hiding this comment.
Can you remove this file from here. I doubt it belong here
| """scan_keys should return an empty list when nothing matches.""" | ||
| keys = redis.scan_keys("nonexistent_pattern_xyz:*") | ||
| assert keys == [] | ||
|
|
| return None | ||
|
|
||
|
|
||
| def provision_instance(spec, replica_index, next_host_port=None): |
There was a problem hiding this comment.
what is this spec ?
What is provider in this spec
| logging.basicConfig(level=logging.INFO) | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
There was a problem hiding this comment.
create a separate util file for this
| text=True, | ||
| ) | ||
|
|
||
| def _ensure_remote_docker(self, host, user=None): |
There was a problem hiding this comment.
assume that the ami you use have the docker image.
Remove all this code.
| ] | ||
| return cmd | ||
|
|
||
| def _run_remote_script(self, host, script, user=None): |
There was a problem hiding this comment.
What shell script is this running?
| sudo systemctl enable --now docker || sudo service docker start | ||
| """.strip() | ||
| return self._run_remote_script(host, script, user) | ||
|
|
There was a problem hiding this comment.
move it out to a utility function
|
|
||
| logger.info("Redis launched on %d node(s).", len(self.redis_containers)) | ||
|
|
||
| def _wait_for_redis(self, redis_client, host, port, timeout=30, interval=1): |
There was a problem hiding this comment.
Move all this into separate files
| security_group_ids: | ||
| - sg-0123456789abcdef0 | ||
| transport: ssm | ||
| endpoint_url: |
There was a problem hiding this comment.
What is transport ?
What is endpoint_url
| transport: ssm | ||
| endpoint_url: | ||
| key_name: ventis-key | ||
| ssm_document_name: AWS-RunShellScript |
|
Just wanted to add a quick explanation of what I did in the commit "added changes". I removed all the functions that loaded the image into containers, under the assumption that the image was already loaded in. Added a util folder and files to the functions that were mentioned in the comments above to be moved, removed the ssm code that was unnecessary, and deleted the test that bled through from a future PR. In the future I'll add more detailed PR messages to make this code review process easier |
📝 WalkthroughWalkthroughThe change adds local and EC2 runtime backends, centralizes instance lifecycle and routing through ChangesRuntime and deployment orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GlobalController
participant InstanceManager
participant LocalRuntime
participant EC2Runtime
participant Redis
GlobalController->>InstanceManager: ensure_instances(agent_specs)
InstanceManager->>LocalRuntime: provision and bootstrap local instance
InstanceManager->>EC2Runtime: provision and bootstrap EC2 instance
InstanceManager->>Redis: persist instance records
InstanceManager->>Redis: publish routing snapshot
GlobalController->>InstanceManager: list_instances for health and cleanup
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
21-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the test suite in CI.
This job only runs Ruff and Ty, so runtime regressions covered by
tests/can still merge with a green CI result. Add at least the unit-test suite after dependency installation.🤖 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 @.github/workflows/ci.yml around lines 21 - 33, Add a CI step after “Install dependencies for Ty” that runs the repository’s unit-test suite from tests/ using the project’s configured test command, ensuring dependencies are installed before execution.README.md (1)
23-23: 🩺 Stability & Availability | 🔵 TrivialDocument how remote images are provisioned.
ventis buildcreates images on the deploy machine, while this note requires those images to already exist on remote hosts. Add the required registry push/pull or image transfer step, including the expected image tags, so a fresh EC2 deployment is reproducible.🤖 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 `@README.md` at line 23, Update the README installation/deployment note to document how images built by `ventis build` are provisioned to remote EC2 hosts: specify the registry push and remote pull or image transfer workflow, include the expected image tags, and ensure the steps support a reproducible fresh deployment.
🤖 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 @.github/workflows/ci.yml:
- Around line 13-14: Update the “Checkout code” step using actions/checkout@v4
to set persist-credentials to false, preventing the checkout token from being
retained in local Git configuration.
- Around line 8-10: The workflow lacks explicit least-privilege GITHUB_TOKEN
permissions. Add a top-level permissions block near the workflow definition with
contents: read, ensuring the test job and future jobs default to read-only
repository access.
In `@examples/config/global_controller.ec2_smoke.yaml`:
- Line 3: Update the smoke configuration comment to state that the EC2 launcher
uses a prebuilt image containing the agent code, without a bind mount or
entrypoint override; remove the incorrect “generic agent image + bind-mounted
project code” assumption.
In `@tests/run_tests.sh`:
- Around line 8-12: The test environment directory must use an unpredictable,
safely created temporary path. In tests/run_tests.sh, define TEST_DIR with
mktemp -d using the /tmp/ventis_test_env.XXXXXX template, then use TEST_DIR
wherever the test environment path is needed instead of constructing it with $$.
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 113-143: Wrap the post-`run_instances` provisioning flow in
`_runtime.py`—including the instance waiter, IP polling, and host validation—in
a try/except block. On any exception after `instance_id` is obtained, call
`client.terminate_instances(InstanceIds=[instance_id])`, then re-raise the
original exception; preserve successful return behavior and avoid cleanup before
the instance ID exists.
- Around line 129-151: Update the host selection in the EC2 runtime provisioning
logic to prefer the instance’s PublicIpAddress and fall back to
PrivateIpAddress, so controller health checks, routing, and Redis connections
use the controller-reachable address. Apply this consistently to the host value
stored in the record and preserve the existing missing-address error handling.
- Around line 94-113: The EC2 launch request in the runtime provisioning flow
lacks an IAM instance profile required for SSM registration. Add a validated
configuration field for the instance role/profile, then update the request
constructed before client.run_instances in the EC2 launch logic to include
IamInstanceProfile using that configured profile.
- Around line 288-304: Update the SSM polling function containing the
get_command_invocation call to catch the client’s InvocationDoesNotExist
exception, sleep briefly, and continue polling until the existing deadline.
Preserve the current success, failure-status, unexpected-status, and timeout
behavior for all other cases.
In `@ventis/controller/cloud_provider_logic/Local/_runtime.py`:
- Line 48: The new runtime ID format in the local runtime must be used by
stale-container cleanup. Update GlobalController._cleanup_stale_containers() to
derive container names through the provider-specific runtime ID logic, such as
the existing runtime ID builder, instead of reconstructing the legacy
ventis-<agent>-<replica> pattern; ensure local cleanup targets ventis-local-…
names.
- Around line 83-84: The workflow branch in the runtime command construction
currently hardcodes host port 8080, causing collisions between workflow
instances. Update the runtime configuration and persistence logic around this
command to allocate a unique host API port per workflow instance and use it in
the “-p” mapping, or add validation that rejects multiple workflows on the same
host if distinct port allocation is unsupported.
In `@ventis/controller/global_controller.py`:
- Around line 477-490: Update the cleanup broadcast logic around the
request-completion handling to track whether every intended instance delivery
succeeds, using the loop in the relevant controller method. Only call redis.srem
for request:completed after all stubs acknowledge Cleanup successfully; if any
delivery raises an exception, retain the request ID for a later retry while
preserving the existing warning logs.
- Around line 139-152: _get_replica_placements must not default EC2 replica
hosts to localhost or derive Redis topology from pre-provision configuration.
Resolve each EC2 replica’s host and Redis endpoint from the provisioned
instance/runtime metadata, or require and consistently use one explicit shared
Redis endpoint; update callers and health polling to publish and connect to that
same endpoint.
- Around line 480-482: The synchronous Cleanup RPC in the background loop lacks
a deadline and can block indefinitely. Update the call in the controller’s
cleanup delivery logic to pass a configurable timeout (for example, via the gRPC
timeout argument), and catch timeout/RPC failures so the cleanup is recorded as
a failed delivery and the loop continues.
- Around line 181-185: Update the no-policy branch in the policy-loading
function to return an empty rule list instead of None. Ensure the caller
`_load_and_write_policies()` can safely execute `len(rules)` when the policy
file is absent, while preserving the existing log message and early-return
behavior.
- Around line 512-515: Update the remote command construction in the controller
method containing ssh_target and remote_cmd to shell-quote every cmd argument
(for example with shlex.quote) before joining, preserving the sudo prefix for
Docker commands. Also validate or safely quote the user and host components used
to build ssh_target so configuration-derived values cannot inject SSH or shell
options.
In `@ventis/controller/instance_manager.py`:
- Around line 34-55: Canonicalize the provider name before generating instance
IDs, Redis keys, and agent associations. In the instance provisioning flow, use
the canonical provider returned or resolved by _provider_runtime() rather than
the raw agent_spec provider value, and ensure _instance_id(), _instance_key(),
and _add_instance_to_agent() receive that canonical value consistently.
In `@ventis/controller/local_controller.py`:
- Around line 338-343: The INFO log in the already-resolved future handling
should not include raw existing_result data. Update the logger.info call to emit
only safe metadata such as the future identifier, endpoint, and payload size, or
redact the result; keep full payload details restricted to controlled DEBUG
logging.
- Around line 299-303: When no endpoint is found in the routing-failure branch
of the controller, persist a terminal failure result/error for the relevant
future and notify the origin callback before returning; update the logic around
the endpoint lookup and Future.value polling path so callers do not wait
indefinitely.
In `@ventis/ventis_context.py`:
- Around line 12-14: Update the get_request_id() docstring to state that it
returns an empty string when no request ID is set, matching the current getattr
fallback; alternatively, change the fallback to None only if that is the
intended API contract.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 21-33: Add a CI step after “Install dependencies for Ty” that runs
the repository’s unit-test suite from tests/ using the project’s configured test
command, ensuring dependencies are installed before execution.
In `@README.md`:
- Line 23: Update the README installation/deployment note to document how images
built by `ventis build` are provisioned to remote EC2 hosts: specify the
registry push and remote pull or image transfer workflow, include the expected
image tags, and ensure the steps support a reproducible fresh deployment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01c24bc3-c688-46f5-81dc-d44dc45674af
📒 Files selected for processing (37)
.dockerignore.github/workflows/ci.yml.gitignoreREADME.mdexamples/agents/finance_agent.pyexamples/agents/market_agent.pyexamples/agents/vllm_agent.pyexamples/config/global_controller.ec2_smoke.yamlexamples/config/global_controller.yamlpyproject.tomlrequirements.txttests/README.mdtests/run_tests.shtests/test_cli.pytests/test_instance_manager_runtime.pytests/test_integration.pytests/test_performance.pytests/test_redis_utils.pytests/test_runtime_ec2.pytests/test_stateful_affinity.pyventis/cli.pyventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/cloud_provider_logic/Local/_runtime.pyventis/controller/global_controller.pyventis/controller/instance_manager.pyventis/controller/local_controller.pyventis/controller/local_controller_frontend.pyventis/controller/utils/__init__.pyventis/controller/utils/agent_specs.pyventis/controller/utils/redis_utils.pyventis/deploy.pyventis/future.pyventis/stub_generator.pyventis/templates/agents/vllm_agent.pyventis/templates/config/global_controller.yamlventis/utils/redis_client.pyventis/ventis_context.py
💤 Files with no reviewable changes (1)
- tests/test_stateful_affinity.py
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable checkout credential persistence.
Set persist-credentials: false on actions/checkout; otherwise the token remains in the local Git configuration and can be exposed by later commands or generated artifacts.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 13-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml around lines 13 - 14, Update the “Checkout code”
step using actions/checkout@v4 to set persist-credentials to false, preventing
the checkout token from being retained in local Git configuration.
Source: Linters/SAST tools
| @@ -0,0 +1,33 @@ | |||
| # EC2 variant of the main example config: | |||
| # - exactly 2 agent instances | |||
| # - generic agent image + bind-mounted project code | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the smoke-config deployment assumption.
The EC2 launcher runs ventis-marketresearchagent with no bind mount and does not use entrypoint; this sample requires a prebuilt image containing the agent code, not a generic image plus mounted project source.
🤖 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 `@examples/config/global_controller.ec2_smoke.yaml` at line 3, Update the smoke
configuration comment to state that the EC2 launcher uses a prebuilt image
containing the agent code, without a bind mount or entrypoint override; remove
the incorrect “generic agent image + bind-mounted project code” assumption.
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" &> /dev/null && pwd)" | ||
|
|
||
| echo ">> 0. Running small pytest suite..." | ||
| python3 -m pytest "$SCRIPT_DIR" | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create the test directory with mktemp -d.
The /tmp/ventis_test_env_$$ path is predictable and can be pre-created or raced by another local user, potentially redirecting writes or cleanup. Use TEST_DIR="$(mktemp -d /tmp/ventis_test_env.XXXXXX)" instead.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 12-12: Building a temp file path in a world-writable directory from the PID ($$) or `` is predictable and racy: an attacker can pre-create or guess the name and win a symlink/race attack. Use mktemp (e.g. `f=$(mktemp)` or `f=$(mktemp /tmp/myapp.XXXXXX)`) so the kernel atomically creates a unique, unpredictable file.
Context: "/tmp/ventis_test_env_$$"
Note: [CWE-377] Insecure Temporary File.
(tmp-file-pid-name-bash)
🤖 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 `@tests/run_tests.sh` around lines 8 - 12, The test environment directory must
use an unpredictable, safely created temporary path. In tests/run_tests.sh,
define TEST_DIR with mktemp -d using the /tmp/ventis_test_env.XXXXXX template,
then use TEST_DIR wherever the test environment path is needed instead of
constructing it with $$.
Source: Linters/SAST tools
| request = { | ||
| "ImageId": cfg["ami_id"], | ||
| "InstanceType": spec["instance_type"], | ||
| "SubnetId": cfg["subnet_id"], | ||
| "SecurityGroupIds": cfg["security_group_ids"], | ||
| "MinCount": 1, | ||
| "MaxCount": 1, | ||
| "TagSpecifications": [ | ||
| { | ||
| "ResourceType": "instance", | ||
| "Tags": [ | ||
| {"Key": "Name", "Value": f"ventis-{agent_name}-{replica_index}"} | ||
| ], | ||
| } | ||
| ], | ||
| } | ||
| if cfg.get("key_name"): | ||
| request["KeyName"] = cfg["key_name"] | ||
|
|
||
| response = client.run_instances(**request) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline ventis/controller/cloud_provider_logic/EC2/_runtime.py --view expanded || true
printf '\n== Relevant searches ==\n'
rg -n "IamInstanceProfile|send_command|get_command_invocation|run_instances|SSM|Systems Manager|instance profile|managed node" ventis/controller/cloud_provider_logic README.md . || true
printf '\n== Target file excerpt ==\n'
nl -ba ventis/controller/cloud_provider_logic/EC2/_runtime.py | sed -n '1,220p'Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 6091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== EC2 runtime excerpt ==\n'
sed -n '84,140p' ventis/controller/cloud_provider_logic/EC2/_runtime.py
printf '\n== EC2 runtime SSM excerpt ==\n'
sed -n '214,312p' ventis/controller/cloud_provider_logic/EC2/_runtime.py
printf '\n== Repo search for instance profile / IAM config ==\n'
rg -n "instance_profile|IamInstanceProfile|iam instance profile|iam_instance|role_arn|role name|ssm.*profile|Systems Manager.*profile|managed node" ventis tests README.md . || true
printf '\n== Test file outline ==\n'
ast-grep outline tests/test_runtime_ec2.py --view expanded || true
printf '\n== Config-related files ==\n'
git ls-files | rg 'EC2|ec2|config|runtime' || trueRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 7645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tests/test_runtime_ec2.py relevant sections ==\n'
sed -n '101,360p' tests/test_runtime_ec2.py
printf '\n== example controller config ==\n'
sed -n '1,240p' examples/config/global_controller.ec2_smoke.yaml
printf '\n== template controller config ==\n'
sed -n '1,240p' ventis/templates/config/global_controller.yaml
printf '\n== any SSM/EC2 bootstrap docs in README ==\n'
rg -n "SSM|Systems Manager|EC2|instance profile|iam_instance_profile|run_instances" README.md examples/config ventis/templates/config || trueRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 12092
Attach an IAM instance profile to EC2 launches.
ventis/controller/cloud_provider_logic/EC2/_runtime.py:94-113 always boots via SSM, but run_instances never passes IamInstanceProfile. Add a validated config field for the instance role/profile and include it in the launch request so new instances can register as SSM managed nodes before bootstrap runs.
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 94 -
113, The EC2 launch request in the runtime provisioning flow lacks an IAM
instance profile required for SSM registration. Add a validated configuration
field for the instance role/profile, then update the request constructed before
client.run_instances in the EC2 launch logic to include IamInstanceProfile using
that configured profile.
| ssh_target = f"{user}@{host}" if user else host | ||
| remote_cmd = " ".join(cmd) | ||
| if cmd and cmd[0] == "docker": | ||
| remote_cmd = f"sudo {remote_cmd}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Quote remote Docker arguments before passing them to SSH.
cmd contains configuration-derived values such as agent names and resource settings. " ".join(cmd) is evaluated by the remote shell, so shell metacharacters can execute arbitrary commands on the EC2 host. Construct the remote command with shell-safe quoting and validate host/user values.
Proposed fix
+import shlex
+
- remote_cmd = " ".join(cmd)
+ remote_cmd = shlex.join(cmd)📝 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.
| ssh_target = f"{user}@{host}" if user else host | |
| remote_cmd = " ".join(cmd) | |
| if cmd and cmd[0] == "docker": | |
| remote_cmd = f"sudo {remote_cmd}" | |
| ssh_target = f"{user}@{host}" if user else host | |
| remote_cmd = shlex.join(cmd) | |
| if cmd and cmd[0] == "docker": | |
| remote_cmd = f"sudo {remote_cmd}" |
🤖 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 `@ventis/controller/global_controller.py` around lines 512 - 515, Update the
remote command construction in the controller method containing ssh_target and
remote_cmd to shell-quote every cmd argument (for example with shlex.quote)
before joining, preserving the sudo prefix for Docker commands. Also validate or
safely quote the user and host components used to build ssh_target so
configuration-derived values cannot inject SSH or shell options.
Source: Linters/SAST tools
| agent_name = agent_spec["name"] | ||
| provider = agent_spec.get("provider", "local") | ||
| runtime = self._provider_runtime(provider) | ||
| self.controller.containers.setdefault(agent_name, []) | ||
|
|
||
| runtime.validate_config() | ||
|
|
||
| for replica_index in range(int(agent_spec.get("replicas", 1))): | ||
| instance_id = self._instance_id(provider, agent_name, replica_index) | ||
| key = self._instance_key(provider, agent_name, replica_index) | ||
| instance = self.redis.hgetall(key) | ||
|
|
||
| if not instance: | ||
| provisioned = runtime.provision_instance( | ||
| agent_spec, replica_index, self._next_host_port | ||
| ) | ||
| instance = runtime.bootstrap_instance( | ||
| provisioned, agent_spec, replica_index | ||
| ) | ||
| self._write_instance(instance) | ||
|
|
||
| self._add_instance_to_agent(agent_name, instance_id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Canonicalize provider names before deriving Redis IDs.
_provider_runtime() accepts case-insensitive providers, but IDs use the raw value. For provider: ec2 or LOCAL, the runtime writes a canonical provider record while the agent set stores a differently-cased ID; list_instances() then cannot retrieve it. This leaves routing empty and can reprovision duplicate EC2 instances.
Proposed fix
- provider = agent_spec.get("provider", "local")
+ raw_provider = agent_spec.get("provider", "local")
+ provider = {"local": "local", "ec2": "EC2"}.get(
+ str(raw_provider).lower()
+ )
+ if provider is None:
+ raise ValueError(f"Unsupported provider: {raw_provider}")📝 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.
| agent_name = agent_spec["name"] | |
| provider = agent_spec.get("provider", "local") | |
| runtime = self._provider_runtime(provider) | |
| self.controller.containers.setdefault(agent_name, []) | |
| runtime.validate_config() | |
| for replica_index in range(int(agent_spec.get("replicas", 1))): | |
| instance_id = self._instance_id(provider, agent_name, replica_index) | |
| key = self._instance_key(provider, agent_name, replica_index) | |
| instance = self.redis.hgetall(key) | |
| if not instance: | |
| provisioned = runtime.provision_instance( | |
| agent_spec, replica_index, self._next_host_port | |
| ) | |
| instance = runtime.bootstrap_instance( | |
| provisioned, agent_spec, replica_index | |
| ) | |
| self._write_instance(instance) | |
| self._add_instance_to_agent(agent_name, instance_id) | |
| agent_name = agent_spec["name"] | |
| raw_provider = agent_spec.get("provider", "local") | |
| provider = {"local": "local", "ec2": "EC2"}.get( | |
| str(raw_provider).lower() | |
| ) | |
| if provider is None: | |
| raise ValueError(f"Unsupported provider: {raw_provider}") | |
| runtime = self._provider_runtime(provider) | |
| self.controller.containers.setdefault(agent_name, []) | |
| runtime.validate_config() | |
| for replica_index in range(int(agent_spec.get("replicas", 1))): | |
| instance_id = self._instance_id(provider, agent_name, replica_index) | |
| key = self._instance_key(provider, agent_name, replica_index) | |
| instance = self.redis.hgetall(key) | |
| if not instance: | |
| provisioned = runtime.provision_instance( | |
| agent_spec, replica_index, self._next_host_port | |
| ) | |
| instance = runtime.bootstrap_instance( | |
| provisioned, agent_spec, replica_index | |
| ) | |
| self._write_instance(instance) | |
| self._add_instance_to_agent(agent_name, instance_id) |
🤖 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 `@ventis/controller/instance_manager.py` around lines 34 - 55, Canonicalize the
provider name before generating instance IDs, Redis keys, and agent
associations. In the instance provisioning flow, use the canonical provider
returned or resolved by _provider_runtime() rather than the raw agent_spec
provider value, and ensure _instance_id(), _instance_key(), and
_add_instance_to_agent() receive that canonical value consistently.
| if not endpoint: | ||
| logger.error("No endpoint found for service '%s' in routing table.", service) | ||
| logger.error( | ||
| "No endpoint found for service '%s' in routing table.", service | ||
| ) | ||
| return |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Resolve the future before returning on routing failure.
When no endpoint exists, this branch only logs and returns. Future.value() polls future:{future_id}:result, so the caller can wait indefinitely because neither Redis nor the origin callback receives a terminal result. Persist a failure result/error and notify origin before returning.
🤖 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 `@ventis/controller/local_controller.py` around lines 299 - 303, When no
endpoint is found in the routing-failure branch of the controller, persist a
terminal failure result/error for the relevant future and notify the origin
callback before returning; update the logic around the endpoint lookup and
Future.value polling path so callers do not wait indefinitely.
| logger.info( | ||
| "Future %s already resolved, pushing value %s to %s", | ||
| value, | ||
| existing_result, | ||
| endpoint, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log raw workflow results at INFO level.
existing_result may contain user or financial data. Log only metadata (future ID, endpoint, payload size) or redact the value; reserve detailed payload logging for controlled debug diagnostics.
🤖 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 `@ventis/controller/local_controller.py` around lines 338 - 343, The INFO log
in the already-resolved future handling should not include raw existing_result
data. Update the logger.info call to emit only safe metadata such as the future
identifier, endpoint, and payload size, or redact the result; keep full payload
details restricted to controlled DEBUG logging.
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 (1)
ventis/controller/cloud_provider_logic/EC2/_runtime.py (1)
258-284: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDrop
-itfrom the remote Docker run
controller._run_cmd()runs this over SSH without a TTY, sodocker run -d -it ...will fail withthe input device is not a TTYduring bootstrap. Since this path tears down the instance on error, remove the interactive flags for this detached container.🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 258 - 284, Remove the "-it" arguments from the Docker command assembled in the remote container startup logic before calling controller._run_cmd(), leaving the detached "-d" mode and all other options unchanged.
♻️ Duplicate comments (2)
ventis/controller/cloud_provider_logic/EC2/_runtime.py (2)
151-182: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvisioning failures still leak a running, billable instance.
This was flagged previously and remains unresolved: after
run_instancessucceeds, a waiter timeout/error,describe_instancesexception, or the "no reachable IP"RuntimeError(line 178-181) all exit without terminatinginstance_id. The instance keeps running and is never persisted for later cleanup.🐛 Proposed fix
response = client.run_instances(**request) instance_id = response["Instances"][0]["InstanceId"] runtime_id = f"ventis-ec2-{agent_name.lower()}-{replica_index}--{instance_id}" - client.get_waiter("instance_running").wait(InstanceIds=[instance_id]) - - deadline = time.time() + cfg.get("public_ip_timeout", 120) - instance = None - while time.time() < deadline: - ... - if not host: - raise RuntimeError( - f"EC2 instance {instance_id} does not have a reachable IP address." - ) + try: + client.get_waiter("instance_running").wait(InstanceIds=[instance_id]) + + deadline = time.time() + cfg.get("public_ip_timeout", 120) + instance = None + while time.time() < deadline: + ... + if not host: + raise RuntimeError( + f"EC2 instance {instance_id} does not have a reachable IP address." + ) + except Exception: + client.terminate_instances(InstanceIds=[instance_id]) + raise🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 151 - 182, Ensure all failures after run_instances in the provisioning flow clean up the created instance. Wrap the waiter, IP polling, and host validation logic following instance_id creation in exception handling that calls client.terminate_instances with instance_id before re-raising, including waiter/describe_instances errors and the no-reachable-IP RuntimeError; preserve the original exception and avoid masking cleanup failures.
173-177: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStill prefers the private IP over the public IP.
Also flagged previously and still unresolved. This matters more now than before: bootstrap uses direct SSH into the host (
_bootstrap_instance), and the example/template configs document inbound SSH (port 22) from the controller's source IP — implying the controller reaches instances externally. If so,PrivateIpAddresswill not be reachable and SSH bootstrap will hang/fail.Proposed fix
host = ( - instance.get("PrivateIpAddress") or instance.get("PublicIpAddress") + instance.get("PublicIpAddress") or instance.get("PrivateIpAddress") if instance else None )🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 173 - 177, Update the host selection in the runtime logic to prefer instance.get("PublicIpAddress") and fall back to instance.get("PrivateIpAddress"), preserving the None result when no instance exists. Verify that _bootstrap_instance receives this externally reachable address for direct SSH connections.
🧹 Nitpick comments (2)
tests/test_runtime_ec2.py (2)
89-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTemp key directory is never cleaned up.
self.key_dir = tempfile.mkdtemp()insetUphas no matching removal intearDown, leaking a temp directory per test run.🧹 Proposed fix
def tearDown(self): self.session_patch.stop() ec2_runtime._controller = self.original_controller ec2_runtime.DEFAULT_SSH_KEY_PATH = self.original_key_path + shutil.rmtree(self.key_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runtime_ec2.py` around lines 89 - 129, Clean up the temporary SSH key directory created in the test setup. Update tearDown to remove self.key_dir using an appropriate temporary-directory cleanup method, while preserving the existing session patch, controller, and key-path restoration steps.
222-241: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBootstrap SSH/docker execution is fully mocked — the TTY/
-itissue flagged in_runtime.pywould pass CI undetected.
controller._run_cmdis aMagicMockreturningreturncode=0, so this test validates call shape/args but never exercises the real subprocess/SSH path where thedocker run -d -itcommand would actually fail. Consider a lower-level test (or integration test) that runs the constructed command through the real_run_cmd/subprocesspath (or at least asserts the constructedcmdlist doesn't include-itfor a detached run) to catch this class of regression.🤖 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 `@tests/test_runtime_ec2.py` around lines 222 - 241, Strengthen test_bootstrap_uses_ssh_user so it verifies detached Docker execution cannot use an interactive TTY: inspect the constructed docker command and assert it excludes “-it” (or specifically exercises the real _run_cmd/subprocess path without mocking it). Preserve the existing SSH-user and invocation assertions while adding coverage that would fail if _bootstrap_instance reintroduces “-it” for docker run -d.
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 118-125: Handle missing EC2 instance types explicitly in
provision_instance: either add instance_type values to the VllmAgent and
Workflow entries in the example configuration, or preferably validate the field
before constructing the request and raise a descriptive ValueError consistent
with the existing required EC2 configuration checks, rather than allowing
spec["instance_type"] to raise KeyError.
---
Outside diff comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 258-284: Remove the "-it" arguments from the Docker command
assembled in the remote container startup logic before calling
controller._run_cmd(), leaving the detached "-d" mode and all other options
unchanged.
---
Duplicate comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Around line 151-182: Ensure all failures after run_instances in the
provisioning flow clean up the created instance. Wrap the waiter, IP polling,
and host validation logic following instance_id creation in exception handling
that calls client.terminate_instances with instance_id before re-raising,
including waiter/describe_instances errors and the no-reachable-IP RuntimeError;
preserve the original exception and avoid masking cleanup failures.
- Around line 173-177: Update the host selection in the runtime logic to prefer
instance.get("PublicIpAddress") and fall back to
instance.get("PrivateIpAddress"), preserving the None result when no instance
exists. Verify that _bootstrap_instance receives this externally reachable
address for direct SSH connections.
---
Nitpick comments:
In `@tests/test_runtime_ec2.py`:
- Around line 89-129: Clean up the temporary SSH key directory created in the
test setup. Update tearDown to remove self.key_dir using an appropriate
temporary-directory cleanup method, while preserving the existing session patch,
controller, and key-path restoration steps.
- Around line 222-241: Strengthen test_bootstrap_uses_ssh_user so it verifies
detached Docker execution cannot use an interactive TTY: inspect the constructed
docker command and assert it excludes “-it” (or specifically exercises the real
_run_cmd/subprocess path without mocking it). Preserve the existing SSH-user and
invocation assertions while adding coverage that would fail if
_bootstrap_instance reintroduces “-it” for docker run -d.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d5a3232-b896-4df9-9857-b502d112b0fa
📒 Files selected for processing (9)
.github/workflows/ci.ymlexamples/config/global_controller.ec2_smoke.yamlexamples/config/global_controller.yamlsession-manager-plugin.pkgtests/test_runtime_ec2.pyventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/global_controller.pyventis/templates/config/global_controller.yamlventis/ventis_context.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- examples/config/global_controller.yaml
- ventis/controller/global_controller.py
| def provision_instance(spec, replica_index, next_host_port=None): | ||
| """Launch one EC2 instance for an agent replica and wait for its IPs.""" | ||
| cfg, client = _aws_clients() | ||
| pubkey = _ensure_ssh_keypair() | ||
| agent_name = spec["name"] | ||
| request = { | ||
| "ImageId": cfg["ami_id"], | ||
| "InstanceType": spec["instance_type"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
spec["instance_type"] is required but the example template omits it for some agents.
provision_instance does spec["instance_type"] with no fallback. In ventis/templates/config/global_controller.yaml, VllmAgent and Workflow both set provider: EC2 but don't define instance_type, so provisioning those agents will raise KeyError instead of a clear config error.
🐛 Proposed fix
- "InstanceType": spec["instance_type"],
+ "InstanceType": spec.get("instance_type") or cfg.get("default_instance_type"),Alternatively, add instance_type to the template entries and raise a descriptive ValueError if it's missing, similar to the required EC2 config fields check.
📝 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.
| def provision_instance(spec, replica_index, next_host_port=None): | |
| """Launch one EC2 instance for an agent replica and wait for its IPs.""" | |
| cfg, client = _aws_clients() | |
| pubkey = _ensure_ssh_keypair() | |
| agent_name = spec["name"] | |
| request = { | |
| "ImageId": cfg["ami_id"], | |
| "InstanceType": spec["instance_type"], | |
| def provision_instance(spec, replica_index, next_host_port=None): | |
| """Launch one EC2 instance for an agent replica and wait for its IPs.""" | |
| cfg, client = _aws_clients() | |
| pubkey = _ensure_ssh_keypair() | |
| agent_name = spec["name"] | |
| request = { | |
| "ImageId": cfg["ami_id"], | |
| "InstanceType": spec.get("instance_type") or cfg.get("default_instance_type"), |
🤖 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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 118 -
125, Handle missing EC2 instance types explicitly in provision_instance: either
add instance_type values to the VllmAgent and Workflow entries in the example
configuration, or preferably validate the field before constructing the request
and raise a descriptive ValueError consistent with the existing required EC2
configuration checks, rather than allowing spec["instance_type"] to raise
KeyError.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
get_request_id()now defaults to an empty string.Documentation
Tests