# Project Review — 2026-08-21 (1)

## Prior Reviews Summary

> Based on the last 3 review files analysed in Phase 0.

### Still Open (carried forward)
None.

### Resolved Since Last Review
None.

---

## Phase 1: Code Quality

### SOLID
- `session.cmd_up` / `_bind_access` orchestrate cloud, SSH, bootstrap, provision, snapshot, notify — god-flow (`src/gpu_rent/session.py`).
- Docs name separate modules (`civitai_seed`, `models_push`, `git_seed`, `watchdog`, `reconcile`) that do not exist as packages; logic is folded into `provision.py`, `sync_files.py`, `tunnel.py`, `cli.status`.
- `Config` dataclass is a wide bag of unrelated settings (auth, paths, idle, autocomplete, civitai).

### Performance
- `push_tree` / `pull_tree`: per-file `remote_sha256` + `put_file` each open a new SSH (`sync_files.py`, `ssh_ops.py`).
- Every `up` re-runs full `bootstrap.sh` (apt-get) via `_bind_access` even when already bootstrapped.
- `scan_pools` on every `up`: extra Keystone authorize + per-flavor HTTP extra_specs (ThreadPool 16).
- Tunnel watchdog: `connect()` / Keystone authorize every ~30s.

### Correctness & Bugs
- `wait_ssh` aborts after 3 `AuthenticationException`s (~15s) while cloud-init may still be injecting keys (`ssh_ops.py` ~114–120).
- `cmd_stop` leaves `bootstrapped=True` (`session.py` ~397–400) → next ACTIVE path mis-labeled / wrong branch when FIP cleared.
- `phase` set to `ready_cloud` before bootstrap finishes (`session.py` ~344–346).
- Application credential created without role/access_rules restriction (`idle_killer.py` ~52–57) vs architecture “delete/shelve only”.
- `GIT_TOKEN` embedded in git remote URL and never stripped (`clone_ext.py` `with_token` / clone).
- SG rules accumulate old CIDRs; never prune (`cloud.py` `ensure_security_group`).
- Session tests out of sync: no `bootstrapped`+FIP setup for “second GPU”; `run_bootstrap` mock missing `update=` (`tests/test_session.py`).

### Code Quality
- State machine phases in docs (`bootstrapping`, `seeding_*`, `waiting_ui`) never written by code (only `idle` / `provisioning` / `ready_*`).
- `os_client.connect` reports `app_version="0.1.0"` while package is `0.2.0`.
- Duplicated SwarmUI busy/Idle polling logic in `ready.py` and `remote/idle_killer.py`.

---

## Phase 2: Logical Consistency

### Domain & Application Layer
- Architecture table invents module names; actual layout is flatter (`cli` → `session`/`doctor`/`tunnel` → `cloud`/`provision`/`ssh_ops`).

### Data Flow
- Config sources: `.env` (dotenv override=False) then `gpu-rent.vars` (override empty only) — OK.
- Volume IDs: `state` preferred over `BOOT_VOLUME_ID` / `DATA_VOLUME_ID` env — OK; AZ not re-validated on reuse.

### State Management
- `bootstrapped` not cleared on `stop`.
- `bootstrapped` True only at end of `_bind_access`; failure mid-bind leaves `ready_cloud` + `bootstrapped=False`.
- Marker files on VM (`/opt/swarmui/.gpu-rent-bootstrapped`) independent of local `state.bootstrapped`.

### Consistency
- Error handling: mostly `GpuRentError` / `CloudError`; some soft-log (`wait_backend_idle`, snapshot, idle-killer arm).
- Civitai bad token is **blocking** in doctor → blocks `up` even if user only needs default SwarmUI models.

---

## Phase 3: UI/UX (CLI)

### Usability
- `up` always prints full doctor table (noisy).
- Invalid/expired `CIVITAI_API_TOKEN` blocks entire session start.
- `destroy` has no `--no-pull` (always respects `PULL_OUTPUT` via `cmd_stop`).

### Visual & Consistency
- N/A (CLI). Mixed RU/EN messages by design.

### Interaction & Feedback
- Confirm on `up` without `--yes` is good; `--yes` skips.
- Tunnel Ctrl+C messaging is clear.
- `logs` only tails cloud-init + `is-active`, not `journalctl -u swarmui` as docs imply.

### Accessibility
- N/A.

---

## Full exploration notes (sections 1–10)

See chat report for narrative. Tasks below are actionable.

---

## Tasks

- [x] 1. [Bug] Soften `wait_ssh` early auth abort (retry until timeout / distinguish “sshd up, keys not ready”) — `src/gpu_rent/ssh_ops.py` line 114
- [x] 2. [Bug] Clear `bootstrapped=False` (and align phase) in `cmd_stop` — `src/gpu_rent/session.py` line 397
- [x] 3. [Security] Restrict idle-killer application credential (access_rules / roles delete-only) — `src/gpu_rent/idle_killer.py` line 52
- [x] 4. [Security] Strip `GIT_TOKEN` from git `origin` after clone/fetch — `src/gpu_rent/remote/clone_ext.py` line 30
- [x] 5. [Security] Stop accumulating stale SSH SG CIDRs; replace or prune old /32 — `src/gpu_rent/cloud.py` line 154
- [x] 6. [Security] Revisit default `GPU_RENT_SSH_CIDR=0.0.0.0/0` in `env.example` — `env.example` line 16
- [x] 7. [Performance] Reuse one SSH/SFTP session in `push_tree` / `pull_tree` / hash checks — `src/gpu_rent/sync_files.py` line 15
- [x] 8. [Performance] Skip full bootstrap (or apt) when VM already marked bootstrapped — `src/gpu_rent/session.py` line 87
- [x] 9. [Logic] Do not set `phase=ready_cloud` before `_bind_access` succeeds — `src/gpu_rent/session.py` line 344
- [x] 10. [Logic] Make doctor Civitai failure non-blocking when seed is optional — `src/gpu_rent/doctor.py` line 271
- [x] 11. [CodeQuality] Fix `tests/test_session.py` mocks (`bootstrapped`+FIP; `run_bootstrap(..., update=)`) — `tests/test_session.py` line 40
- [x] 12. [UX] `logs` should include `journalctl -u swarmui` as docs claim — `src/gpu_rent/cli.py` line 404
- [x] 13. [Logic] Update `docs/architecture.md` module names, phases, questionary, hold `--minutes` wording — `docs/architecture.md` line 37

## Closure notes (2026-08-21)

- `wait_ssh`: auth streak give-up after 180s continuous AuthenticationException (not 3 attempts).
- Idle-killer: access_rules for GET/DELETE server; fallback without rules if Keystone rejects.
- Bootstrap light path: `GPU_RENT_BOOTSTRAP_LIGHT=1` skips apt when marker present.
- Also fixed `os_client` `app_version` → package `__version__` (noted in Phase 1, not a numbered task).