Civ 010 refactor ssh utilities - #606
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/ssh_lib.py" line_range="1-5" />
<code_context>
+from ssh.client import ( # noqa: F401 — re-exports for backward compatibility
+ generate_ssh_key_pair,
+ wait_for_host_ssh_up,
+ add_ssh_keys_to_instances,
+)
+
import os
</code_context>
<issue_to_address>
**issue (bug_risk):** The imported SSH functions are immediately shadowed by the legacy definitions later in `lib/ssh_lib.py`, so backward-compatible callers continue using the old implementations instead of the refactored `ssh.client` functions. The new `subprocess.run` behavior and type-safe implementations are therefore not active for production callers importing `lib.ssh_lib`.
**Suggested fix:** Remove the duplicate legacy definitions or rename the re-exported imports so the imported functions remain bound.
</issue_to_address>
### Comment 2
<location path="core/metadata.py" line_range="73-80" />
<code_context>
+ return json.load(f)
+
+
+def write_instances_json(
+ instances: dict[str, InstanceMetadata],
+ path: str,
+) -> None:
+ document = {key: inst.to_dict() for key, inst in instances.items()}
+
+ schema = _load_schema()
+ jsonschema.validate(document, schema)
+
+ with open(path, 'w') as f:
</code_context>
<issue_to_address>
**issue (broader_impact):** The new metadata writer and schema validation are never used by the production flow: `CloudImageValidator` still calls the legacy raw `json.dump` and `generate_instances_ssh_config` implementations. Production instance metadata therefore bypasses the typed conversion and JSON Schema enforcement introduced here.
**Triggers:** When running the normal `CloudImageValidator.deploy_infrastructure` path.
**Suggested fix:** Update the production validator to construct `InstanceMetadata` values and call `write_instances_json` and `write_ssh_config`.
</issue_to_address>
### Comment 3
<location path="core/metadata.py" line_range="80" />
<code_context>
+ document = {key: inst.to_dict() for key, inst in instances.items()}
+
+ schema = _load_schema()
+ jsonschema.validate(document, schema)
+
+ with open(path, 'w') as f:
</code_context>
<issue_to_address>
**issue (bug_risk):** The schema validation does not validate the type of `image`, because the schema's `image` property has no `type` or equivalent constraint. `write_instances_json` therefore accepts invalid runtime values such as integers, lists, or null for `InstanceMetadata.image`, despite `InstanceMetadata.from_dict` requiring a string or dictionary.
**Triggers:** When an `InstanceMetadata` object is constructed with an invalid non-string, non-dictionary `image` value.
**Suggested fix:** Add an `anyOf` constraint requiring `image` to be either a string or an object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| from ssh.client import ( # noqa: F401 — re-exports for backward compatibility | ||
| generate_ssh_key_pair, | ||
| wait_for_host_ssh_up, | ||
| add_ssh_keys_to_instances, | ||
| ) |
There was a problem hiding this comment.
issue (bug_risk): The imported SSH functions are immediately shadowed by the legacy definitions later in lib/ssh_lib.py, so backward-compatible callers continue using the old implementations instead of the refactored ssh.client functions. The new subprocess.run behavior and type-safe implementations are therefore not active for production callers importing lib.ssh_lib.
Suggested fix: Remove the duplicate legacy definitions or rename the re-exported imports so the imported functions remain bound.
There was a problem hiding this comment.
The re-exports are at lines at the end of file, after the legacy definitions at lines 11–123. AFAIK Python executes module-level statements top to bottom, so the re-exports overwrite the legacy names not the other way around. I believe we can dismiss this one.
There was a problem hiding this comment.
You’re right—the re-exports are at the end of the module, so they execute after the legacy definitions and rebind those three names to ssh.client. My comment incorrectly assumed the imports were at the top based on the displayed hunk. This issue can be dismissed.
| def write_instances_json( | ||
| instances: dict[str, InstanceMetadata], | ||
| path: str, | ||
| ) -> None: | ||
| document = {key: inst.to_dict() for key, inst in instances.items()} | ||
|
|
||
| schema = _load_schema() | ||
| jsonschema.validate(document, schema) |
There was a problem hiding this comment.
issue (broader_impact): The new metadata writer and schema validation are never used by the production flow: CloudImageValidator still calls the legacy raw json.dump and generate_instances_ssh_config implementations. Production instance metadata therefore bypasses the typed conversion and JSON Schema enforcement introduced here.
Triggers: When running the normal CloudImageValidator.deploy_infrastructure path.
Suggested fix: Update the production validator to construct InstanceMetadata values and call write_instances_json and write_ssh_config.
There was a problem hiding this comment.
Intentionally out of scope for this task.
There was a problem hiding this comment.
Understood—if wiring the new utilities into CloudImageValidator.deploy_infrastructure is intentionally out of scope, I’ll treat this as a follow-up rather than a blocker for this PR. Please track it separately, since the normal deployment path will continue to use the legacy JSON and SSH-config writers and therefore won’t receive the new validation/refactoring until that integration is made.
| document = {key: inst.to_dict() for key, inst in instances.items()} | ||
|
|
||
| schema = _load_schema() | ||
| jsonschema.validate(document, schema) |
There was a problem hiding this comment.
issue (bug_risk): The schema validation does not validate the type of image, because the schema's image property has no type or equivalent constraint. write_instances_json therefore accepts invalid runtime values such as integers, lists, or null for InstanceMetadata.image, despite InstanceMetadata.from_dict requiring a string or dictionary.
Triggers: When an InstanceMetadata object is constructed with an invalid non-string, non-dictionary image value.
Suggested fix: Add an anyOf constraint requiring image to be either a string or an object.
There was a problem hiding this comment.
@sshmulev This one is valid. We have found a gap here. The schema is more permissive than the dataclass. Please do not fix this in this PR, we will create a follow up to touch on the schema file again.
A fix could be pretty simple, we can simply add an anyOf to the image definition in the schema, e.g.:
"image": {
"anyOf": [{"type": "string"}, {"type": "object"}],
"description": "..."
}Just a suggestion, we can figure out how to handle this in a follow up task.
There was a problem hiding this comment.
Thanks for confirming. I agree this is a schema gap rather than an issue to address in this PR, so I’ll leave the implementation unchanged. The follow-up should add the anyOf constraint for image and ideally a regression test covering invalid scalar, array, and null values so schema validation stays aligned with InstanceMetadata.from_dict.
f6b0196 to
2ffddcc
Compare
2caf2ab to
7dc74cc
Compare
Port generate_ssh_key_pair, wait_for_host_ssh_up, and add_ssh_keys_to_instances from lib/ssh_lib.py to ssh/client.py with full type hints and subprocess.run replacing os.system calls. Add re-exports in lib/ssh_lib.py for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2ffddcc to
a8d51e1
Compare
Move re-exports to bottom of file so they shadow the original definitions, and suppress F811/F401/E402 warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
F-X64
left a comment
F-X64
left a comment
There was a problem hiding this comment.
Very solid implementation. None of the issues I found are blockers so please go ahead and merge, we can add the findings as small fixup tasks if you prefere.
| print(f"[{instance_address}] Public SSH key(s) copied successfully!") | ||
|
|
||
|
|
||
| from ssh.client import generate_ssh_key_pair as generate_ssh_key_pair # noqa: E402, F811, F401 |
There was a problem hiding this comment.
Thanks for fixing the linter errors!
| f"Timeout while waiting for {host_address} to be SSH-ready ({timeout_seconds} seconds)." | ||
| ) | ||
| print("AWS: Check if this account has the appropiate inbound rules for this region") | ||
| exit(1) |
There was a problem hiding this comment.
This was already part of the original code so the transfer is correct.
We could however use this to improve logging a bit, e.g. raising a TimeoutError or RuntimeError instead of just printing it out.
| exit(1) | |
| raise TimeoutError( | |
| f"Timed out waiting for {host_address} SSH after {timeout_seconds}s" | |
| ) |
We wouldn't need to worry about the current behavior of cloud_image-validator.py as TimeoutError would still propagate up and crash as it currently does (we will catch these errors instead of simply exiting in the following tasks).
| from ssh.client import ( # noqa: F401 — re-exports for backward compatibility | ||
| generate_ssh_key_pair, | ||
| wait_for_host_ssh_up, | ||
| add_ssh_keys_to_instances, | ||
| ) |
There was a problem hiding this comment.
The re-exports are at lines at the end of file, after the legacy definitions at lines 11–123. AFAIK Python executes module-level statements top to bottom, so the re-exports overwrite the legacy names not the other way around. I believe we can dismiss this one.
| def write_instances_json( | ||
| instances: dict[str, InstanceMetadata], | ||
| path: str, | ||
| ) -> None: | ||
| document = {key: inst.to_dict() for key, inst in instances.items()} | ||
|
|
||
| schema = _load_schema() | ||
| jsonschema.validate(document, schema) |
There was a problem hiding this comment.
Intentionally out of scope for this task.
| document = {key: inst.to_dict() for key, inst in instances.items()} | ||
|
|
||
| schema = _load_schema() | ||
| jsonschema.validate(document, schema) |
There was a problem hiding this comment.
@sshmulev This one is valid. We have found a gap here. The schema is more permissive than the dataclass. Please do not fix this in this PR, we will create a follow up to touch on the schema file again.
A fix could be pretty simple, we can simply add an anyOf to the image definition in the schema, e.g.:
"image": {
"anyOf": [{"type": "string"}, {"type": "object"}],
"description": "..."
}Just a suggestion, we can figure out how to handle this in a follow up task.
2 participants
No description provided.