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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (24)
📝 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
| 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.
| 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.
| 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
5fdc3dc to
e30fbe1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ventis/controller/cloud_provider_logic/EC2/_runtime.py (2)
242-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
DEFAULT_SSH_KEY_PATHinstead of re-expanding the literal path.key = os.path.expanduser("~/.ssh/ventis_ec2")duplicates the module constant; referencingDEFAULT_SSH_KEY_PATHkeeps the keypair path single-sourced (and consistent with_ensure_ssh_keypair).♻️ Suggested change
- key = os.path.expanduser("~/.ssh/ventis_ec2") + key = DEFAULT_SSH_KEY_PATH🤖 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` at line 242, Replace the duplicated expanded SSH key path in the surrounding runtime logic with the existing DEFAULT_SSH_KEY_PATH constant, matching the path used by _ensure_ssh_keypair and keeping the keypair location single-sourced.
246-253: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer avoiding
shell=Truefor the image transfer. Thedocker save | ssh ... docker loadpipeline interpolatesimage,key,ssh_user, andhostinto a shell string. These are config/AWS-derived rather than request-derived, so this is a hardening concern rather than an active RCE, but a hostname/user with shell metacharacters would still break or alter the command. Run the pipe via an explicit shell arg list orshlex.quote()the interpolated values.🤖 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 246 - 253, Harden the image transfer subprocess in the EC2 runtime by avoiding unescaped interpolation in the shell pipeline. Update the subprocess.run call to invoke the pipeline through an explicit shell argument list or quote image, key, ssh_user, and host with shlex.quote(), while preserving the existing docker save to SSH docker load behavior.Source: Linters/SAST tools
ventis/controller/cloud_provider_logic/EC2/README.md (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a space after the list dash so these render as bullets.
-Securityand-Passwordlesswon't be parsed as list items in Markdown.📝 Suggested change
-Security group allowing port 50051, 6379, 8080, and 22 communication within the security group -Passwordless ssh set up. +- Security group allowing ports 50051, 6379, 8080, and 22 within the security group +- Passwordless SSH set up.🤖 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/README.md` around lines 6 - 7, Update the two list entries in the README by adding a space after each leading dash, so “Security” and “Passwordless” render as Markdown bullet items.
🤖 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 `@session-manager-plugin.pkg`:
- Line 1: Remove the unused vendored session-manager-plugin.pkg installer from
version control, since no repository references require it. Do not add an
on-demand installation step unless it is documented and necessary.
In `@ventis/future.py`:
- Around line 135-137: Update the error check in LocalController’s future value
flow to use presence semantics: change the conditional around
redis.hget("error") so any non-None stored error, including an empty string,
raises RuntimeError. Preserve the existing polling behavior when the Redis field
is absent.
---
Nitpick comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Line 242: Replace the duplicated expanded SSH key path in the surrounding
runtime logic with the existing DEFAULT_SSH_KEY_PATH constant, matching the path
used by _ensure_ssh_keypair and keeping the keypair location single-sourced.
- Around line 246-253: Harden the image transfer subprocess in the EC2 runtime
by avoiding unescaped interpolation in the shell pipeline. Update the
subprocess.run call to invoke the pipeline through an explicit shell argument
list or quote image, key, ssh_user, and host with shlex.quote(), while
preserving the existing docker save to SSH docker load behavior.
In `@ventis/controller/cloud_provider_logic/EC2/README.md`:
- Around line 6-7: Update the two list entries in the README by adding a space
after each leading dash, so “Security” and “Passwordless” render as Markdown
bullet items.
🪄 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: 4146d862-a7d7-4245-9a15-a35400d1761e
📒 Files selected for processing (16)
.github/workflows/ci.ymlexamples/config/global_controller.ec2_smoke.yamlexamples/config/global_controller.yamlsession-manager-plugin.pkgtests/test_error_propagation.pytests/test_instance_manager_runtime.pytests/test_runtime_ec2.pyventis/controller/cloud_provider_logic/EC2/README.mdventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/global_controller.pyventis/controller/instance_manager.pyventis/controller/local_controller.pyventis/future.pyventis/templates/README.mdventis/templates/config/global_controller.yamlventis/ventis_context.py
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/ci.yml
- ventis/ventis_context.py
- examples/config/global_controller.ec2_smoke.yaml
- examples/config/global_controller.yaml
- ventis/templates/config/global_controller.yaml
- tests/test_instance_manager_runtime.py
- ventis/controller/instance_manager.py
- ventis/controller/local_controller.py
- ventis/controller/global_controller.py
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: 2
🧹 Nitpick comments (3)
ventis/controller/cloud_provider_logic/EC2/_runtime.py (2)
242-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
DEFAULT_SSH_KEY_PATHinstead of re-expanding the literal path.key = os.path.expanduser("~/.ssh/ventis_ec2")duplicates the module constant; referencingDEFAULT_SSH_KEY_PATHkeeps the keypair path single-sourced (and consistent with_ensure_ssh_keypair).♻️ Suggested change
- key = os.path.expanduser("~/.ssh/ventis_ec2") + key = DEFAULT_SSH_KEY_PATH🤖 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` at line 242, Replace the duplicated expanded SSH key path in the surrounding runtime logic with the existing DEFAULT_SSH_KEY_PATH constant, matching the path used by _ensure_ssh_keypair and keeping the keypair location single-sourced.
246-253: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer avoiding
shell=Truefor the image transfer. Thedocker save | ssh ... docker loadpipeline interpolatesimage,key,ssh_user, andhostinto a shell string. These are config/AWS-derived rather than request-derived, so this is a hardening concern rather than an active RCE, but a hostname/user with shell metacharacters would still break or alter the command. Run the pipe via an explicit shell arg list orshlex.quote()the interpolated values.🤖 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 246 - 253, Harden the image transfer subprocess in the EC2 runtime by avoiding unescaped interpolation in the shell pipeline. Update the subprocess.run call to invoke the pipeline through an explicit shell argument list or quote image, key, ssh_user, and host with shlex.quote(), while preserving the existing docker save to SSH docker load behavior.Source: Linters/SAST tools
ventis/controller/cloud_provider_logic/EC2/README.md (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a space after the list dash so these render as bullets.
-Securityand-Passwordlesswon't be parsed as list items in Markdown.📝 Suggested change
-Security group allowing port 50051, 6379, 8080, and 22 communication within the security group -Passwordless ssh set up. +- Security group allowing ports 50051, 6379, 8080, and 22 within the security group +- Passwordless SSH set up.🤖 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/README.md` around lines 6 - 7, Update the two list entries in the README by adding a space after each leading dash, so “Security” and “Passwordless” render as Markdown bullet items.
🤖 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 `@session-manager-plugin.pkg`:
- Line 1: Remove the unused vendored session-manager-plugin.pkg installer from
version control, since no repository references require it. Do not add an
on-demand installation step unless it is documented and necessary.
In `@ventis/future.py`:
- Around line 135-137: Update the error check in LocalController’s future value
flow to use presence semantics: change the conditional around
redis.hget("error") so any non-None stored error, including an empty string,
raises RuntimeError. Preserve the existing polling behavior when the Redis field
is absent.
---
Nitpick comments:
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Line 242: Replace the duplicated expanded SSH key path in the surrounding
runtime logic with the existing DEFAULT_SSH_KEY_PATH constant, matching the path
used by _ensure_ssh_keypair and keeping the keypair location single-sourced.
- Around line 246-253: Harden the image transfer subprocess in the EC2 runtime
by avoiding unescaped interpolation in the shell pipeline. Update the
subprocess.run call to invoke the pipeline through an explicit shell argument
list or quote image, key, ssh_user, and host with shlex.quote(), while
preserving the existing docker save to SSH docker load behavior.
In `@ventis/controller/cloud_provider_logic/EC2/README.md`:
- Around line 6-7: Update the two list entries in the README by adding a space
after each leading dash, so “Security” and “Passwordless” render as Markdown
bullet items.
🪄 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: 4146d862-a7d7-4245-9a15-a35400d1761e
📒 Files selected for processing (16)
.github/workflows/ci.ymlexamples/config/global_controller.ec2_smoke.yamlexamples/config/global_controller.yamlsession-manager-plugin.pkgtests/test_error_propagation.pytests/test_instance_manager_runtime.pytests/test_runtime_ec2.pyventis/controller/cloud_provider_logic/EC2/README.mdventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/global_controller.pyventis/controller/instance_manager.pyventis/controller/local_controller.pyventis/future.pyventis/templates/README.mdventis/templates/config/global_controller.yamlventis/ventis_context.py
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/ci.yml
- ventis/ventis_context.py
- examples/config/global_controller.ec2_smoke.yaml
- examples/config/global_controller.yaml
- ventis/templates/config/global_controller.yaml
- tests/test_instance_manager_runtime.py
- ventis/controller/instance_manager.py
- ventis/controller/local_controller.py
- ventis/controller/global_controller.py
🛑 Comments failed to post (2)
session-manager-plugin.pkg (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# macOS example — pin to a specific version/checksum in CI or setup docs instead of committing the .pkg curl -o session-manager-plugin.pkg "https://s3.amazonaws.com/session-manager-downloads/plugin/latest/mac/session-manager-plugin.pkg"Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 565
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== tracked files matching session-manager-plugin / ssm ==\n' git ls-files | rg -n 'session-manager-plugin|session manager|session-manager|ssm' || true printf '\n== text references in repo ==\n' rg -n -i 'session[- ]manager[- ]plugin|session manager plugin|ssm session|aws ssm|start-session|ssm' . \ --glob '!*.pkg' --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' --glob '!*.gif' --glob '!*.pdf' || true printf '\n== file info if present ==\n' if git ls-files --error-unmatch session-manager-plugin.pkg >/dev/null 2>&1; then ls -lh session-manager-plugin.pkg file session-manager-plugin.pkg || true fiRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 531
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== exact filename references ==\n' rg -n --hidden --glob '!*.pkg' --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' --glob '!*.gif' --glob '!*.pdf' 'session-manager-plugin\.pkg' . || true printf '\n== session-manager-plugin text references ==\n' rg -n --hidden --glob '!*.pkg' --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' --glob '!*.gif' --glob '!*.pdf' 'session-manager-plugin' . || true printf '\n== aws ssm / start-session references (word-ish) ==\n' rg -n --hidden --glob '!*.pkg' --glob '!*.png' --glob '!*.jpg' --glob '!*.jpeg' --glob '!*.gif' --glob '!*.pdf' -i '\b(aws ssm|session manager plugin|start-session|ssm)\b' . || trueRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 301
Remove the vendored Session Manager Plugin installer.
session-manager-plugin.pkghas no references in the repo, so it is dead weight in version control. Delete it unless you add a documented on-demand install step 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 `@session-manager-plugin.pkg` at line 1, Remove the unused vendored session-manager-plugin.pkg installer from version control, since no repository references require it. Do not add an on-demand installation step unless it is documented and necessary.ventis/future.py (1)
135-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check for error presence, not truthiness.
LocalController._forward_requeststoresstr(e)for every caught exception, andstr(Exception())can be empty. Withif error:, that failure is ignored, causingvalue()to keep polling or time out instead of raising. Useif error is not 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/future.py` around lines 135 - 137, Update the error check in LocalController’s future value flow to use presence semantics: change the conditional around redis.hget("error") so any non-None stored error, including an empty string, raises RuntimeError. Preserve the existing polling behavior when the Redis field is absent.
iidsample
left a comment
iidsample
left a comment
There was a problem hiding this comment.
Separate Notes:
-
Move to a passwordless AMI for time being
-
Make the port dynamic
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/local_controller.py (1)
497-500: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate forwarding failures upstream in
_forward_request. Preserve the incomingoriginbefore overwritingdata["origin"], then call_send_result_callback(..., error=str(e))on the exception path; otherwise the upstream future can stay pending.🤖 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 497 - 500, Update `_forward_request` to preserve the incoming `origin` before replacing `data["origin"]`, then invoke `_send_result_callback` with the preserved origin and `error=str(e)` when forwarding raises. Retain the existing Redis error recording, but ensure the upstream future is notified on the exception path.
🤖 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/local_controller.py`:
- Around line 398-400: The Redis dependency failure in the local execution flow
must remain an error rather than being converted into result data. Update
_execute_locally and its surrounding result-handling path so the failure is
preserved and propagated via the error field, instead of being stored under
result or sent as a normal result; keep successful execution results unchanged.
---
Outside diff comments:
In `@ventis/controller/local_controller.py`:
- Around line 497-500: Update `_forward_request` to preserve the incoming
`origin` before replacing `data["origin"]`, then invoke `_send_result_callback`
with the preserved origin and `error=str(e)` when forwarding raises. Retain the
existing Redis error recording, but ensure the upstream future is notified on
the exception path.
🪄 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: c19e0ee6-363b-494f-b870-925356b767b7
📒 Files selected for processing (5)
ventis/controller/cloud_provider_logic/EC2/_runtime.pyventis/controller/global_controller.pyventis/controller/instance_manager.pyventis/controller/local_controller.pyventis/templates/config/global_controller.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- ventis/templates/config/global_controller.yaml
- ventis/controller/instance_manager.py
- ventis/controller/cloud_provider_logic/EC2/_runtime.py
- ventis/controller/global_controller.py
| error = self.redis.hget(future_key, "error") | ||
| if error: | ||
| raise RuntimeError(error) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not convert dependency failures into result data.
This raises an error, but _execute_locally catches it and stores Execution failed: ... under result, then sends it as a normal result. Downstream agents can consume the failure string as valid input. Preserve this as an error and propagate it with error=....
🤖 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 398 - 400, The Redis
dependency failure in the local execution flow must remain an error rather than
being converted into result data. Update _execute_locally and its surrounding
result-handling path so the failure is preserved and propagated via the error
field, instead of being stored under result or sent as a normal result; keep
successful execution results unchanged.
…eck test reorganized the instance manager routing logic to make it provider agnostic reorganized the instance manager routing logic to make it provider agnostic removed folder that should be gitignored removed a useless test added changes Added lint/type checking and fixed errors shown by them removed some previous stale testing code removed SSM and added passwordless ssh Cleaned up changed code EC2 path working now cut down some chaff and added readme removed stale package manager
26c0253 to
28cdd86
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
ventis/controller/local_controller.py (2)
300-303: 🩺 Stability & Availability | 🟠 MajorResolve the Future on routing failure.
When no endpoint is found, this branch only logs and returns. The caller can wait indefinitely because no terminal error is persisted or sent to the origin callback. Store and propagate a failure 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 300 - 303, Update the no-endpoint branch in the local controller’s routing flow to persist a terminal failure and propagate it through the origin callback before returning. Keep the existing error log, and ensure callers are notified rather than left waiting indefinitely.
338-343: 🔒 Security & Privacy | 🟠 MajorDo not log raw results at INFO level.
existing_result,serialized, and callback payloads are emitted verbatim and may contain user or workflow data. Log only safe metadata, reserving redacted payloads for controlled DEBUG diagnostics.Also applies to: 450-456, 496-515
🤖 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, Update the INFO-level logging around the resolved future and the related locations in the local controller to remove raw values such as existing_result, serialized data, and callback payloads. Log only safe metadata (for example, identifiers, destinations, or sizes), and keep any redacted payload details limited to controlled DEBUG diagnostics.
🤖 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 `@tests/test_integration.py`:
- Line 13: Update both HTTP requests in the integration test, including the POST
to /main and the other request at the referenced location, to pass an explicit
timeout argument. Use the existing max_wait configuration or an appropriate
finite request-timeout value so neither call can block indefinitely, while
preserving the current request behavior.
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Line 15: Update _userdata() to return the raw shell script instead of
base64-encoding it, remove the now-unused base64 import, and adjust the related
test expectation to assert the plain script passed to UserData.
- Around line 247-253: Update the image transfer subprocess flow around the
docker save/SSH invocation to remove shell=True and avoid interpolating
agent_name, ssh_user, host, or key into a shell command. Run docker save and
argv-based ssh separately, passing the saved image bytes through the SSH process
stdin and preserving the existing sudo docker load behavior.
In `@ventis/controller/global_controller.py`:
- Around line 112-116: Update the host_containers construction in the placements
loop to use the runtime’s canonical container names for stale cleanup: local
runtimes must follow the ventis-local-agent-replica naming scheme instead of
ventis-agent-replica. For EC2 placements, obtain cleanup names through the
runtime metadata rather than constructing them directly. Preserve the existing
Redis container tracking.
- Around line 233-241: Update the node aggregation logic around self.controllers
and nodes so repeated hosts with different redis_port values are rejected
instead of silently retaining the first port. Allow repeated hosts when the
configured ports match, and raise the established validation/configuration error
for conflicts before runtime connections are created.
In `@ventis/controller/instance_manager.py`:
- Line 43: The replica configuration is used inconsistently for integer and
explicit-list forms. In ventis/controller/instance_manager.py lines 43-43,
normalize the replicas value before the loop and iterate using the resulting
placement count/list without calling int() directly on a list. In
ventis/controller/global_controller.py lines 162-174, persist len(replicas) when
the configuration is list-form and int(replicas) otherwise, reusing the
normalized value consistently.
- Around line 48-55: Update the provisioning flow around _write_instance() so
that any metadata persistence exception after bootstrap_instance succeeds
triggers cleanup of the provisioned runtime before propagating the error. Use
the existing runtime cleanup/termination operation, ensure cleanup is attempted
only for the newly provisioned instance, and preserve the normal successful
persistence path.
In `@ventis/controller/local_controller.py`:
- Around line 457-459: Update the exception handling in the execution flow near
the logger.error call so failures are stored in the future’s error field and
propagated through the separate error path, rather than written to the normal
result field. Preserve the existing exception details and ensure downstream
agents receive the failure as an error, not business data.
- Around line 490-492: Update the failure handling around _forward_request so it
preserves the original origin across hops rather than relying on the overwritten
origin value. When a forwarded request fails and has a future_id, send the error
callback to the original owner controller, while retaining the existing
future:{id}.error recording as appropriate.
In `@ventis/future.py`:
- Line 113: Update the synchronous gRPC calls in the future execution flow,
including stub.Execute and the WriteResult call used by _notify_consumers, to
pass explicit timeout values so a stuck peer cannot block indefinitely. Preserve
the existing completion and notification behavior while ensuring
_notify_consumers cannot delay value() forever.
- Around line 135-140: Update _execute_locally() to store caught execution
exceptions in the Redis “error” field instead of “result”, and ensure the
callback payload propagates that same error field. Preserve Future.value()’s
existing error check so failed executions raise RuntimeError rather than
returning a failure string.
---
Duplicate comments:
In `@ventis/controller/local_controller.py`:
- Around line 300-303: Update the no-endpoint branch in the local controller’s
routing flow to persist a terminal failure and propagate it through the origin
callback before returning. Keep the existing error log, and ensure callers are
notified rather than left waiting indefinitely.
- Around line 338-343: Update the INFO-level logging around the resolved future
and the related locations in the local controller to remove raw values such as
existing_result, serialized data, and callback payloads. Log only safe metadata
(for example, identifiers, destinations, or sizes), and keep any redacted
payload details limited to controlled DEBUG diagnostics.
🪄 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: 840415e5-98e6-4e1c-8011-c935272eb4da
📒 Files selected for processing (38)
.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.tomltests/README.mdtests/run_tests.shtests/test_cli.pytests/test_error_propagation.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/README.mdventis/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/README.mdventis/templates/agents/vllm_agent.pyventis/templates/config/global_controller.yamlventis/utils/redis_client.pyventis/ventis_context.py
💤 Files with no reviewable changes (2)
- tests/test_stateful_affinity.py
- ventis/controller/utils/agent_specs.py
🚧 Files skipped from review as they are similar to previous changes (24)
- examples/config/global_controller.ec2_smoke.yaml
- ventis/controller/cloud_provider_logic/EC2/README.md
- tests/test_redis_utils.py
- ventis/templates/README.md
- examples/agents/finance_agent.py
- ventis/templates/agents/vllm_agent.py
- ventis/controller/cloud_provider_logic/Local/_runtime.py
- tests/README.md
- pyproject.toml
- .gitignore
- README.md
- examples/agents/vllm_agent.py
- ventis/deploy.py
- ventis/controller/utils/redis_utils.py
- ventis/ventis_context.py
- tests/run_tests.sh
- ventis/utils/redis_client.py
- examples/config/global_controller.yaml
- tests/test_cli.py
- ventis/controller/local_controller_frontend.py
- tests/test_error_propagation.py
- tests/test_instance_manager_runtime.py
- tests/test_performance.py
- ventis/stub_generator.py
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: 11
♻️ Duplicate comments (2)
ventis/controller/local_controller.py (2)
300-303: 🩺 Stability & Availability | 🟠 MajorResolve the Future on routing failure.
When no endpoint is found, this branch only logs and returns. The caller can wait indefinitely because no terminal error is persisted or sent to the origin callback. Store and propagate a failure 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 300 - 303, Update the no-endpoint branch in the local controller’s routing flow to persist a terminal failure and propagate it through the origin callback before returning. Keep the existing error log, and ensure callers are notified rather than left waiting indefinitely.
338-343: 🔒 Security & Privacy | 🟠 MajorDo not log raw results at INFO level.
existing_result,serialized, and callback payloads are emitted verbatim and may contain user or workflow data. Log only safe metadata, reserving redacted payloads for controlled DEBUG diagnostics.Also applies to: 450-456, 496-515
🤖 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, Update the INFO-level logging around the resolved future and the related locations in the local controller to remove raw values such as existing_result, serialized data, and callback payloads. Log only safe metadata (for example, identifiers, destinations, or sizes), and keep any redacted payload details limited to controlled DEBUG diagnostics.
🤖 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 `@tests/test_integration.py`:
- Line 13: Update both HTTP requests in the integration test, including the POST
to /main and the other request at the referenced location, to pass an explicit
timeout argument. Use the existing max_wait configuration or an appropriate
finite request-timeout value so neither call can block indefinitely, while
preserving the current request behavior.
In `@ventis/controller/cloud_provider_logic/EC2/_runtime.py`:
- Line 15: Update _userdata() to return the raw shell script instead of
base64-encoding it, remove the now-unused base64 import, and adjust the related
test expectation to assert the plain script passed to UserData.
- Around line 247-253: Update the image transfer subprocess flow around the
docker save/SSH invocation to remove shell=True and avoid interpolating
agent_name, ssh_user, host, or key into a shell command. Run docker save and
argv-based ssh separately, passing the saved image bytes through the SSH process
stdin and preserving the existing sudo docker load behavior.
In `@ventis/controller/global_controller.py`:
- Around line 112-116: Update the host_containers construction in the placements
loop to use the runtime’s canonical container names for stale cleanup: local
runtimes must follow the ventis-local-agent-replica naming scheme instead of
ventis-agent-replica. For EC2 placements, obtain cleanup names through the
runtime metadata rather than constructing them directly. Preserve the existing
Redis container tracking.
- Around line 233-241: Update the node aggregation logic around self.controllers
and nodes so repeated hosts with different redis_port values are rejected
instead of silently retaining the first port. Allow repeated hosts when the
configured ports match, and raise the established validation/configuration error
for conflicts before runtime connections are created.
In `@ventis/controller/instance_manager.py`:
- Line 43: The replica configuration is used inconsistently for integer and
explicit-list forms. In ventis/controller/instance_manager.py lines 43-43,
normalize the replicas value before the loop and iterate using the resulting
placement count/list without calling int() directly on a list. In
ventis/controller/global_controller.py lines 162-174, persist len(replicas) when
the configuration is list-form and int(replicas) otherwise, reusing the
normalized value consistently.
- Around line 48-55: Update the provisioning flow around _write_instance() so
that any metadata persistence exception after bootstrap_instance succeeds
triggers cleanup of the provisioned runtime before propagating the error. Use
the existing runtime cleanup/termination operation, ensure cleanup is attempted
only for the newly provisioned instance, and preserve the normal successful
persistence path.
In `@ventis/controller/local_controller.py`:
- Around line 457-459: Update the exception handling in the execution flow near
the logger.error call so failures are stored in the future’s error field and
propagated through the separate error path, rather than written to the normal
result field. Preserve the existing exception details and ensure downstream
agents receive the failure as an error, not business data.
- Around line 490-492: Update the failure handling around _forward_request so it
preserves the original origin across hops rather than relying on the overwritten
origin value. When a forwarded request fails and has a future_id, send the error
callback to the original owner controller, while retaining the existing
future:{id}.error recording as appropriate.
In `@ventis/future.py`:
- Line 113: Update the synchronous gRPC calls in the future execution flow,
including stub.Execute and the WriteResult call used by _notify_consumers, to
pass explicit timeout values so a stuck peer cannot block indefinitely. Preserve
the existing completion and notification behavior while ensuring
_notify_consumers cannot delay value() forever.
- Around line 135-140: Update _execute_locally() to store caught execution
exceptions in the Redis “error” field instead of “result”, and ensure the
callback payload propagates that same error field. Preserve Future.value()’s
existing error check so failed executions raise RuntimeError rather than
returning a failure string.
---
Duplicate comments:
In `@ventis/controller/local_controller.py`:
- Around line 300-303: Update the no-endpoint branch in the local controller’s
routing flow to persist a terminal failure and propagate it through the origin
callback before returning. Keep the existing error log, and ensure callers are
notified rather than left waiting indefinitely.
- Around line 338-343: Update the INFO-level logging around the resolved future
and the related locations in the local controller to remove raw values such as
existing_result, serialized data, and callback payloads. Log only safe metadata
(for example, identifiers, destinations, or sizes), and keep any redacted
payload details limited to controlled DEBUG diagnostics.
🪄 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: 840415e5-98e6-4e1c-8011-c935272eb4da
📒 Files selected for processing (38)
.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.tomltests/README.mdtests/run_tests.shtests/test_cli.pytests/test_error_propagation.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/README.mdventis/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/README.mdventis/templates/agents/vllm_agent.pyventis/templates/config/global_controller.yamlventis/utils/redis_client.pyventis/ventis_context.py
💤 Files with no reviewable changes (2)
- tests/test_stateful_affinity.py
- ventis/controller/utils/agent_specs.py
🚧 Files skipped from review as they are similar to previous changes (24)
- examples/config/global_controller.ec2_smoke.yaml
- ventis/controller/cloud_provider_logic/EC2/README.md
- tests/test_redis_utils.py
- ventis/templates/README.md
- examples/agents/finance_agent.py
- ventis/templates/agents/vllm_agent.py
- ventis/controller/cloud_provider_logic/Local/_runtime.py
- tests/README.md
- pyproject.toml
- .gitignore
- README.md
- examples/agents/vllm_agent.py
- ventis/deploy.py
- ventis/controller/utils/redis_utils.py
- ventis/ventis_context.py
- tests/run_tests.sh
- ventis/utils/redis_client.py
- examples/config/global_controller.yaml
- tests/test_cli.py
- ventis/controller/local_controller_frontend.py
- tests/test_error_propagation.py
- tests/test_instance_manager_runtime.py
- tests/test_performance.py
- ventis/stub_generator.py
🛑 Comments failed to post (11)
tests/test_integration.py (1)
13-13: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound both HTTP calls with request timeouts.
max_waitonly limits polling iterations; either request can block the integration run indefinitely.Proposed fix
- response = requests.post(f"{base_url}/main", json={"ticker": "MSFT"}) + response = requests.post( + f"{base_url}/main", json={"ticker": "MSFT"}, timeout=10 + ) ... - status_res = requests.get(f"{base_url}/status/{req_id}").json() + status_res = requests.get( + f"{base_url}/status/{req_id}", timeout=5 + ).json()Also applies to: 28-28
🤖 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_integration.py` at line 13, Update both HTTP requests in the integration test, including the POST to /main and the other request at the referenced location, to pass an explicit timeout argument. Use the existing max_wait configuration or an appropriate finite request-timeout value so neither call can block indefinitely, while preserving the current request behavior.Source: Linters/SAST tools
ventis/controller/cloud_provider_logic/EC2/_runtime.py (2)
15-15: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file first, then read the relevant slice with line numbers. ast-grep outline ventis/controller/cloud_provider_logic/EC2/_runtime.py --view expanded || true wc -l ventis/controller/cloud_provider_logic/EC2/_runtime.py cat -n ventis/controller/cloud_provider_logic/EC2/_runtime.py | sed -n '1,220p'Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 9162
🌐 Web query:
AWS EC2 RunInstances UserData boto3 base64 encode SDK documentation💡 Result:
When using the AWS SDK for Python (Boto3) to launch an EC2 instance via the run_instances or create_instances methods, you do not need to manually base64-encode the UserData string [1]. The Boto3 library handles the base64-encoding for you automatically before sending the request to the Amazon EC2 API [1]. You simply need to pass the raw string containing your script or configuration (such as a bash script or cloud-init content) directly to the UserData parameter [2][3]. Key technical details: - Do not use the file:// prefix: Unlike the AWS Command Line Interface (CLI), Boto3 does not support the file:// syntax for reading user data files [3]. You must read the file contents into a string variable within your Python code before passing it to the method [2][3]. - Content format: Ensure your UserData string starts with the appropriate interpreter directive (e.g., #!/bin/bash for shell scripts or
#cloud-configfor cloud-init directives) to be parsed correctly by the instance [2]. - Encoding: While the underlying EC2 API strictly requires base64-encoded UserData [4][5], the Boto3 SDK abstracts this requirement away, making manual encoding unnecessary [1]. Example usage: import boto3 # Read script from a file with open('user_data.sh', 'r') as file: user_data_script = file.read # Pass the raw string to UserData ec2 = boto3.client('ec2') ec2.run_instances( ImageId='ami-xxxxxxxx', MinCount=1, MaxCount=1, InstanceType='t2.micro', UserData=user_data_script)Citations:
- 1: boto/boto3#1013
- 2: https://stackoverflow.com/questions/42103469/aws-boto3-python-run-instances-method-not-parsing-userdata-parameter
- 3: https://stackoverflow.com/questions/45861615/amazon-boto3run-instance-with-user-data-flag-has-no-effect-on-newly-created-i
- 4: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_RunInstances.html
- 5: https://docs.aws.amazon.com/goto/boto3/ec2-2016-11-15/RunInstances
Pass raw user-data to
UserData
boto3already base64-encodesUserData, so_userdata()should return the plain shell script and thebase64import can be removed; otherwise the instance receives encoded text and the SSH setup won’t run. Update the test to expect the raw script too.🤖 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` at line 15, Update _userdata() to return the raw shell script instead of base64-encoding it, remove the now-unused base64 import, and adjust the related test expectation to assert the plain script passed to UserData.
247-253: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path path = Path("ventis/controller/cloud_provider_logic/EC2/_runtime.py") lines = path.read_text().splitlines() for start, end in [(220, 280), (1, 220)]: print(f"\n--- {path} lines {start}-{end} ---") for i in range(start, min(end, len(lines)) + 1): print(f"{i:4d}: {lines[i-1]}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 10611
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path path = Path("ventis/controller/cloud_provider_logic/EC2/_runtime.py") lines = path.read_text().splitlines() # Print any helper that shells out via ssh or subprocess in this module for i, line in enumerate(lines, 1): if "def _run_cmd" in line or "shell=True" in line or "ssh_user" in line or "agent_name" in line: print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 1326
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find where EC2 spec/config is validated and where agent names come from. rg -n --hidden --glob '!**/.git/**' \ 'ssh_user|spec\["name"\]|spec\.get\("name"|name\]' \ ventis | sed -n '1,200p'Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 2813
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --glob '!**/.git/**' \ 'ssh_user|spec\["name"\]|spec\.get\("name"|name\]' \ ventis | sed -n '1,220p'Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 2813
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the command runner and agent-spec loading to see whether inputs are sanitized upstream. python3 - <<'PY' from pathlib import Path for p in [ Path("ventis/controller/global_controller.py"), Path("ventis/controller/instance_manager.py"), Path("ventis/controller/cloud_provider_logic/Local/_runtime.py"), ]: if p.exists(): print(f"\n--- {p} ---") lines = p.read_text().splitlines() for i, line in enumerate(lines, 1): if "_run_cmd" in line or 'spec["name"]' in line or "agent_spec" in line or "name" in line and "validate" in line: print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 1955
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for p in [ Path("ventis/controller/global_controller.py"), Path("ventis/controller/instance_manager.py"), Path("ventis/controller/cloud_provider_logic/Local/_runtime.py"), ]: if not p.exists(): continue print(f"\n--- {p} ---") lines = p.read_text().splitlines() for i, line in enumerate(lines, 1): if "_run_cmd" in line or 'spec["name"]' in line or "agent_spec" in line or "validate" in line: print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 2119
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path files = [ Path("ventis/controller/utils/agent_specs.py"), Path("ventis/controller/global_controller.py"), Path("ventis/templates/config/global_controller.yaml"), Path("ventis/controller/cloud_provider_logic/EC2/_runtime.py"), ] for p in files: if not p.exists(): continue print(f"\n--- {p} ---") lines = p.read_text().splitlines() for i, line in enumerate(lines, 1): if any(k in line for k in ["validate", "ssh_user", "agent_name", "name", "ec2", "write_agent_specs", "shell=True", "subprocess.run"]): print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 5753
Remove
shell=Truefrom the image transferventis/controller/cloud_provider_logic/EC2/_runtime.py:247-253
agent_name,ssh_user,host, andkeyare interpolated into a shell string, so a crafted config value can execute commands on the controller host. Use argv-basedsshand pipedocker savethrough stdin instead.🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 247-253: Dynamic command passed to subprocess with shell=True. Use a command list without shell=True, or use shlex.quote() to sanitize input.
(coderabbit.command-injection.python-shell-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 `@ventis/controller/cloud_provider_logic/EC2/_runtime.py` around lines 247 - 253, Update the image transfer subprocess flow around the docker save/SSH invocation to remove shell=True and avoid interpolating agent_name, ssh_user, host, or key into a shell command. Run docker save and argv-based ssh separately, passing the saved image bytes through the SSH process stdin and preserving the existing sudo docker load behavior.Source: Linters/SAST tools
ventis/controller/global_controller.py (2)
112-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the runtime’s canonical container names during stale cleanup.
Current local runtimes are named
ventis-local-<agent>-<replica>, while this removesventis-<agent>-<replica>. Stale containers therefore survive and can block later launches with name conflicts. EC2 cleanup should likewise be delegated through its runtime metadata.🤖 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 112 - 116, Update the host_containers construction in the placements loop to use the runtime’s canonical container names for stale cleanup: local runtimes must follow the ventis-local-agent-replica naming scheme instead of ventis-agent-replica. For EC2 placements, obtain cleanup names through the runtime metadata rather than constructing them directly. Preserve the existing Redis container tracking.
233-241: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject conflicting Redis ports on the same host.
nodes[host]silently retains the first port, while each runtime uses its own configuredredis_port. A later agent on that host can therefore connect to a port where no Redis container was launched.Proposed validation
redis_port = ctrl.get("redis_port", 6379) for host, _port in self._get_replica_placements(ctrl): + existing = nodes.get(host) + if existing and existing["redis_port"] != redis_port: + raise ValueError( + f"Conflicting Redis ports configured for {host}" + ) if host not in nodes:📝 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.for ctrl in self.controllers: user = ctrl.get("user") redis_port = ctrl.get("redis_port", 6379) for host, _port in self._get_replica_placements(ctrl): existing = nodes.get(host) if existing and existing["redis_port"] != redis_port: raise ValueError( f"Conflicting Redis ports configured for {host}" ) if host not in nodes: nodes[host] = { "user": user, "redis_port": redis_port, }🤖 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 233 - 241, Update the node aggregation logic around self.controllers and nodes so repeated hosts with different redis_port values are rejected instead of silently retaining the first port. Allow repeated hosts when the configured ports match, and raise the established validation/configuration error for conflicts before runtime connections are created.ventis/controller/instance_manager.py (2)
43-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize the
replicasconfiguration once before consuming it.Both paths advertise support for explicit replica lists but later assume an integer.
ventis/controller/instance_manager.py#L43-L43: iterate the normalized placement count/list instead of callingint()on a list.ventis/controller/global_controller.py#L162-L174: persistlen(replicas)for list-form configuration andint(replicas)otherwise.📍 Affects 2 files
ventis/controller/instance_manager.py#L43-L43(this comment)ventis/controller/global_controller.py#L162-L174🤖 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` at line 43, The replica configuration is used inconsistently for integer and explicit-list forms. In ventis/controller/instance_manager.py lines 43-43, normalize the replicas value before the loop and iterate using the resulting placement count/list without calling int() directly on a list. In ventis/controller/global_controller.py lines 162-174, persist len(replicas) when the configuration is list-form and int(replicas) otherwise, reusing the normalized value consistently.
48-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back the runtime if metadata persistence fails.
Once bootstrap succeeds, an exception from
_write_instance()leaves a running resource with no Redis record. A retry can then create a duplicate EC2 instance or container.Proposed fix
instance = runtime.bootstrap_instance( provisioned, agent_spec, replica_index ) - self._write_instance(instance) + try: + self._write_instance(instance) + except Exception: + runtime.terminate_instance(instance) + raise📝 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.if not instance: provisioned = runtime.provision_instance( agent_spec, replica_index, self._next_host_port ) instance = runtime.bootstrap_instance( provisioned, agent_spec, replica_index ) try: self._write_instance(instance) except Exception: runtime.terminate_instance(instance) 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/instance_manager.py` around lines 48 - 55, Update the provisioning flow around _write_instance() so that any metadata persistence exception after bootstrap_instance succeeds triggers cleanup of the provisioned runtime before propagating the error. Use the existing runtime cleanup/termination operation, ensure cleanup is attempted only for the newly provisioned instance, and preserve the normal successful persistence path.ventis/controller/local_controller.py (2)
457-459: 🗄️ Data Integrity & Integration | 🟠 Major
Preserve execution failures as errors.
The exception path still writes
Execution failed: ...into the future’s normalresultfield and sends it as valid result data. Store failures undererrorand propagate them separately so downstream agents cannot consume failure text as business data.🤖 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 457 - 459, Update the exception handling in the execution flow near the logger.error call so failures are stored in the future’s error field and propagated through the separate error path, rather than written to the normal result field. Preserve the existing exception details and ensure downstream agents receive the failure as an error, not business data.
490-492: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate relevant files and inspect the controller plus future implementation. git ls-files | rg '^(ventis/controller/local_controller\.py|ventis/.*future.*|ventis/.*controller.*|.*future.*\.py)$' || true echo '--- local_controller.py outline ---' ast-grep outline ventis/controller/local_controller.py --view expanded || true echo '--- future-related search ---' rg -n "future_id|origin|error|callback|terminal|forward_request|_forward_request|Future" ventis -g '*.py' -g '*.md' || true echo '--- local_controller.py around suspected lines ---' nl -ba ventis/controller/local_controller.py | sed -n '430,560p'Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 14185
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path files = [ "ventis/controller/local_controller.py", "ventis/controller/local_controller_frontend.py", "ventis/future.py", ] for path in files: print(f"\n===== {path} =====") lines = Path(path).read_text().splitlines() for start in [250, 370, 470, 1, 120, 180]: if start <= len(lines): end = min(len(lines), start + 80) if path.endswith("future.py") and start not in (1, 120, 180): continue if path.endswith("local_controller_frontend.py") and start not in (1, 120, 180): continue if path.endswith("local_controller.py") and start not in (250, 370, 470): continue print(f"\n--- lines {start}-{end} ---") for i in range(start, end + 1): print(f"{i:4d}: {lines[i-1]}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 24894
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re for path in ["ventis/deploy.py", "ventis/templates/config/global_controller.yaml", "ventis/controller/utils/redis_utils.py", "ventis/controller/global_controller.py"]: p = Path(path) if not p.exists(): continue print(f"\n===== {path} =====") lines = p.read_text().splitlines() for i, line in enumerate(lines, 1): if re.search(r"REDIS|redis|host|port|future:|request:", line, re.I): print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 13743
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re targets = [ "ventis/controller/cloud_provider_logic/Local/_runtime.py", "ventis/controller/cloud_provider_logic/EC2/_runtime.py", "ventis/templates/README.md", "ventis/controller/local_controller.py", "ventis/controller/global_controller.py", ] for path in targets: p = Path(path) if not p.exists(): continue print(f"\n===== {path} =====") lines = p.read_text().splitlines() for i, line in enumerate(lines, 1): if re.search(r"VENTIS_REDIS_HOST|VENTIS_REDIS_PORT|LocalController|redis|future|origin|controller", line, re.I): print(f"{i:4d}: {line}") PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 18929
Propagate forward failures to the original owner.
_forward_request()overwritesoriginon each hop, so a downstream failure only writesfuture:{id}.errorto the current controller’s Redis. In a multi-hop route, the first caller can miss the terminal failure signal; keep the original origin and send an error callback back to it 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 `@ventis/controller/local_controller.py` around lines 490 - 492, Update the failure handling around _forward_request so it preserves the original origin across hops rather than relying on the overwritten origin value. When a forwarded request fails and has a future_id, send the error callback to the original owner controller, while retaining the existing future:{id}.error recording as appropriate.ventis/future.py (2)
113-113: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail printf '\n== ventis/future.py line count ==\n' wc -l ventis/future.py printf '\n== ventis/future.py around referenced lines ==\n' sed -n '80,140p' ventis/future.py printf '\n---\n' sed -n '180,220p' ventis/future.py printf '\n== search for Execute / WriteResult definitions and call sites ==\n' rg -n "Execute|WriteResult" -S .Repository: CanyonCodeCoreAI/canyoncodecore
Length of output: 6732
🏁 Script executed:
set -euo pipefail printf '\n== local_controller.py around Execute/WriteResult client calls ==\n' sed -n '450,525p' ventis/controller/local_controller.py printf '\n== local_controller_frontend.py server implementations ==\n' sed -n '1,120p' ventis/controller/local_controller_frontend.py printf '\n== search for timeout/deadline usage in repo ==\n' rg -n "timeout=|deadline|grpc\.insecure_channel|Execute\(request|WriteResult\(request" ventis tests -SRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 11257
🏁 Script executed:
set -euo pipefail printf '\n== ventis/future.py value() and nearby methods ==\n' sed -n '140,215p' ventis/future.pyRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 2979
Add deadlines to the synchronous gRPC calls
stub.Execute()andstub.WriteResult()can block forever on a stuck peer. Since_notify_consumers()runs beforevalue()returns, a hungWriteResultcan delay a completed future indefinitely. Pass explicittimeout=values here, or move the callback off the return path.🤖 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/future.py` at line 113, Update the synchronous gRPC calls in the future execution flow, including stub.Execute and the WriteResult call used by _notify_consumers, to pass explicit timeout values so a stuck peer cannot block indefinitely. Preserve the existing completion and notification behavior while ensuring _notify_consumers cannot delay value() forever.
135-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== ventis/future.py lines 1-260 ==\n' nl -ba ventis/future.py | sed -n '1,260p' printf '\n== Search for _execute_locally and related error/result writes ==\n' rg -n "_execute_locally|error|result|deadline|timeout|grpc" ventis -S printf '\n== Any gRPC stub calls in ventis ==\n' rg -n "grpc\..*|stub\." ventis -SRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 245
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path('ventis/future.py') text = p.read_text() for needle in ['def _execute_locally', 'def _poll_redis', 'error', 'result', 'Execution failed']: print(f'\n## {needle}') for i, line in enumerate(text.splitlines(), 1): if needle in line: start = max(1, i-15) end = min(len(text.splitlines()), i+35) for j in range(start, end+1): print(f'{j:4d}: {text.splitlines()[j-1]}') break print('\n## rg matches') import subprocess, shlex subprocess.run(['rg', '-n', r'_execute_locally|_poll_redis|Execution failed|error|result|deadline|timeout', 'ventis', '-S'], check=False) PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 23299
Write execution failures to
error._execute_locally()still stores caught exceptions inresult, soFuture.value()returns a failure string instead of raising. Write the exception intoerrorand propagate that field in the callback payload too.🤖 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/future.py` around lines 135 - 140, Update _execute_locally() to store caught execution exceptions in the Redis “error” field instead of “result”, and ensure the callback payload propagates that same error field. Preserve Future.value()’s existing error check so failed executions raise RuntimeError rather than returning a failure string.
2 participants
Summary by CodeRabbit
get_request_id()now defaults to an empty string when unset.