CIV-010: Refactor SSH utilities

[sshmulev]

No description provided.

[sourcery-ai]

Hey - I've found 4 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 (broader_impact):** The imported client functions are overwritten by the legacy function definitions later in `lib/ssh_lib.py`, so backward-compatible callers still execute the old `os.system` implementations instead of the refactored `ssh.client` functions. The new subprocess-based behavior is therefore never used through the existing `lib.ssh_lib` API.

**Suggested fix:** Remove the duplicate legacy definitions or rename them so the imported client functions remain the module-level exports.
</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 typed metadata writer, SSH configuration writer, and schema validation are not wired into `CloudImageValidator`; the existing execution path still calls `CloudImageValidator._write_instances_to_json()` and `lib.ssh_lib.generate_instances_ssh_config()`. Normal runs therefore continue writing raw instance dictionaries without schema validation and do not use the new consolidated module.

**Triggers:** When running the existing `CloudImageValidator` workflow.

**Suggested fix:** Update the validator to convert instances with `InstanceMetadata.from_dict()` and call `write_instances_json()`, `write_ssh_config()`, and `set_civ_env_vars()` from the new module.
</issue_to_address>

### Comment 3
<location path="ssh/client.py" line_range="46-50" />
<code_context>
+    start_time = time.time()
+    while time.time() < start_time + timeout_seconds:
+        tick = time.time()
+        result = subprocess.run(
+            ["ssh-keyscan", host_address],
+            capture_output=True,
+            timeout=10,
+        )
+        if result.returncode == 0:
+            print(f"{host_address} SSH is up! ({time.time() - start_time} seconds)")
</code_context>
<issue_to_address>
**issue (bug_risk):** A hung `ssh-keyscan` raises `subprocess.TimeoutExpired`, which is not caught, so the wait function aborts immediately instead of retrying until `timeout_seconds` expires or reporting its normal timeout result.

**Triggers:** When `ssh-keyscan` does not complete within its 10-second subprocess timeout.

**Suggested fix:** Catch `subprocess.TimeoutExpired` and treat it as a failed probe so the loop can continue or reach the configured overall timeout.

```suggestion
        try:
            result = subprocess.run(
                ["ssh-keyscan", host_address],
                capture_output=True,
                timeout=10,
            )
        except subprocess.TimeoutExpired:
            continue
```
</issue_to_address>

### Comment 4
<location path="ssh/client.py" line_range="100-102" />
<code_context>
+        capture_output=True,
+    )
+
+    success = result.returncode == 0
+
+    assert success, f"[{instance_address}] ERROR: Could not copy public SSH key(s)"
+    print(f"[{instance_address}] Public SSH key(s) copied successfully!")
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Failures in `_copy_team_ssh_keys_to_instance` occur inside worker threads, and `add_ssh_keys_to_instances()` only joins those threads without collecting their exceptions; a failed SSH copy therefore prints a thread traceback while the caller returns normally and proceeds as if all keys were installed.

**Triggers:** When any instance's SSH command returns a nonzero status.

**Suggested fix:** Capture worker exceptions or results and raise an aggregate error after joining all threads; do not rely on an assertion in the worker for error propagation.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (broader_impact): The imported client functions are overwritten by the legacy function definitions later in lib/ssh_lib.py, so backward-compatible callers still execute the old os.system implementations instead of the refactored ssh.client functions. The new subprocess-based behavior is therefore never used through the existing lib.ssh_lib API.

Suggested fix: Remove the duplicate legacy definitions or rename them so the imported client functions remain the module-level exports.

[sourcery-ai]

issue (broader_impact): The new typed metadata writer, SSH configuration writer, and schema validation are not wired into CloudImageValidator; the existing execution path still calls CloudImageValidator._write_instances_to_json() and lib.ssh_lib.generate_instances_ssh_config(). Normal runs therefore continue writing raw instance dictionaries without schema validation and do not use the new consolidated module.

Triggers: When running the existing CloudImageValidator workflow.

Suggested fix: Update the validator to convert instances with InstanceMetadata.from_dict() and call write_instances_json(), write_ssh_config(), and set_civ_env_vars() from the new module.

[sourcery-ai]

issue (bug_risk): A hung ssh-keyscan raises subprocess.TimeoutExpired, which is not caught, so the wait function aborts immediately instead of retrying until timeout_seconds expires or reporting its normal timeout result.

Triggers: When ssh-keyscan does not complete within its 10-second subprocess timeout.

Suggested fix: Catch subprocess.TimeoutExpired and treat it as a failed probe so the loop can continue or reach the configured overall timeout.

Suggested change

	result = subprocess.run(
	["ssh-keyscan", host_address],
	capture_output=True,
	timeout=10,
	)
	try:
	result = subprocess.run(
	["ssh-keyscan", host_address],
	capture_output=True,
	timeout=10,
	)
	except subprocess.TimeoutExpired:
	continue

[sourcery-ai]

issue (bug_risk): Failures in _copy_team_ssh_keys_to_instance occur inside worker threads, and add_ssh_keys_to_instances() only joins those threads without collecting their exceptions; a failed SSH copy therefore prints a thread traceback while the caller returns normally and proceeds as if all keys were installed.

Triggers: When any instance's SSH command returns a nonzero status.

Suggested fix: Capture worker exceptions or results and raise an aggregate error after joining all threads; do not rely on an assertion in the worker for error propagation.