Code review findings: 15 defects from /code-review of #32 (005-credential-broker)

This issue collects the 15 confirmed/plausible defects surfaced by a high-effort code review (5 finder angles + 1 verifier + 1 sweep) over the credential-broker work in #32 / branch `005-credential-broker`.

Most-severe first. Each entry is independently actionable; consider splitting into per-finding issues before triage if a single thread gets unwieldy.

---

## 1. Showstopper — `broker_install` URL `v0.1.0` returns 404 today

- **File**: `ansible/roles/broker_install/defaults/main.yml:7`
- **Failure**: `curl -sI -L https://github.com/get2knowio/remo-broker/releases/download/v0.1.0/...` returns HTTP/2 404 for both the binary and `.sha256`. Every `remo {aws,hetzner,incus,proxmox} create` now includes `broker_install` unconditionally, so the very first user to upgrade hits a play failure during their first create.
- **Fix sketch**: Coordinate with `get2knowio/remo-broker` to cut v0.1.0 (see companion remo-broker prompt). Gate the role on \`REMO_BROKER_BACKEND\` being set until then.

## 2. Security — mint exceptions leak the freshly-minted token via `{payload!r}`

- **File**: `src/remo_cli/providers/broker.py:86` (`_onepassword_mint`) and `:146` (`_vault_mint`)
- **Failure**: 1Password / Vault response returns the token under an unexpected key (schema drift, future API version). Token is present in \`payload\` but extractor misses it; \`raise BackendError(f\"...{payload!r}\")\` formats the entire payload — including the live token — into the exception message. \`providers/hetzner.py:215\` then prints it via \`print_error(f\"Bootstrap minting failed: {exc}\")\`. Token ends up in shell scrollback / CI logs / terminal recording.
- **Fix**: Never format \`payload\` into the exception message. Log only field-presence info (\"missing 'token_id'; saw keys: [...]\").

## 3. Showstopper — `lookup('pipe', 'fnox get …')` hard-fails when fnox absent

- **File**: `ansible/group_vars/all.yml:4` (also `ansible/roles/aws_server/defaults/main.yml:16-17`, `ansible/aws_teardown.yml:26-27`)
- **Failure**: Pipe lookups evaluate eagerly during variable resolution. If `fnox` is not on PATH or the key isn't set, Ansible raises before any user-facing validation task can fire. Previously \`lookup('env', ...)\` returned empty gracefully and a friendly-error \`assert\` produced the install-pointer message.
- **Fix**: Wrap the pipe call in a default and surface a friendly error in a pre-flight task. Example: \`lookup('pipe', 'fnox get hetzner_api_token 2>/dev/null || true') | default('', true)\`.

## 4. Subsystem dead — `@cli.result_callback` never fires because subcommands `sys.exit()`

- **File**: `src/remo_cli/cli/main.py:18`
- **Failure**: Every subcommand in `cli/providers/*.py`, `cli/audit.py`, `cli/rotate.py`, `cli/init.py` ends with `sys.exit(rc)`. SystemExit propagates past Click's \`MultiCommand.invoke\`, so the result_callback never runs. Consequence: \`overdue_reminders()\` (T083a passive overdue-rotation warning) AND the pre-existing passive update-check are dead code in practice. Tests cover \`overdue_reminders()\` directly, hiding the dead-hook bug.
- **Fix**: Either move the post-command hook to a Click `result_callback` that returns from subcommands (refactor all `sys.exit(rc)` → `return rc`), or wrap commands in a `try/except SystemExit` that runs the hook before re-raising. Add a CLI integration test that asserts the hook actually fires.

## 5. Destructive — `_aws_sm_revoke` deletes the per-developer IAM role shared across instances

- **File**: `src/remo_cli/providers/broker.py:195`
- **Failure**: \`_ensure_broker_instance_role\` names the role by \`dev_id\` alone, so two \`remo aws create\` calls share the role + profile. When the user destroys instance A, \`_aws_sm_revoke\` calls \`_attach_broker_deny_all_policy\` then \`_delete_broker_instance_role\` against that shared role. Deny-all immediately breaks IMDS credentials on instance B/C/…; the delete then removes the role permanently. No \`list-instance-profiles-for-role\` guard.
- **Fix**: Make the role per-instance (or check for in-use instance profiles before tearing down). Document the per-developer-vs-per-instance scoping decision in `contracts/bootstrap-delivery.md`.

## 6. FR-020 silently broken — Hetzner label keys reject `:`

- **File**: `src/remo_cli/providers/hetzner.py:228` (write site); `:172` (silent swallow)
- **Failure**: Hetzner Cloud's label-key regex disallows \`:\`. PUTs of \`remo:bootstrap-token-id\` / \`remo:rotation-cadence-days\` / \`remo:last-rotation-at\` return HTTP 400. \`_set_server_label\` catches \`urllib.error.URLError\` silently → label never lands → \`_lookup_token_id\` returns None → \`revoke_before_destroy\` skips revocation → backend token orphaned forever. Same defect makes \`_read_rotation_metadata\` permanently return (7, None, None), marking every Hetzner host overdue.
- **Fix**: Use `_`/`.`/`-` instead of `:` in label keys (e.g., `remo_bootstrap_token_id`). Audit the entire `remo:*` label namespace; update tests + reader.

## 7. FR-020 silently broken — `revoke_before_destroy` treats `None` from `_lookup_token_id` as success

- **File**: `src/remo_cli/core/broker_revoke.py:33` (and `:84` for the lookup that swallows)
- **Failure**: \`_lookup_token_id\` wraps the entire Hetzner GET in \`except Exception: return None\`, returning the same value when the API is unreachable as when no label was ever set. Caller then \`return True\` and destroy proceeds. Asymmetric: a successful lookup followed by a revoke failure correctly blocks with exit 5, but a lookup failure silently succeeds → a Hetzner outage during destroy = guaranteed leaked token.
- **Fix**: Distinguish \"no token to revoke\" (return None) from \"lookup failed\" (raise; caller honors `--force`). Drop the bare except.

## 8. FR-020 only enforced on Hetzner — AWS/Incus/Proxmox `destroy` skip the hook

- **File**: `src/remo_cli/providers/aws.py:665`, `incus.py:247`, `proxmox.py:356`
- **Failure**: \`grep -rn revoke_before_destroy src/remo_cli/providers/\` shows the only caller is `hetzner.py:344`. The other three providers' \`destroy()\` paths run the teardown playbook / API call without ever invoking \`broker.revoke_bootstrap_token\`. The exit-5 contract in `contracts/cli-surface.md` is unreachable on three of four providers.
- **Fix**: Wire \`revoke_before_destroy\` into every provider's `destroy()`. See deferred T074 in `specs/005-credential-broker/tasks.md`.

## 9. Service starts in `failed` state — handler always fires on fresh install

- **File**: `ansible/roles/broker_install/handlers/main.yml:2` (paired with `tasks/main.yml:70,100,107`)
- **Failure**: The template renders on every play (changed=true on first install), notifying \`restart remo-broker\`. The handler unconditionally \`state: restarted\`s the unit at end-of-play, but with no token on a fresh install the daemon exits non-zero → systemd transitions to \`failed\`. The post-install verify task (line 107) is gated on \`broker_token_present\` and skips, so Ansible reports green. Operator sees success; broker is broken.
- **Fix**: Either gate the handler on \`broker_token_present | default(false)\`, or make the verify task unconditional with a friendly skip-message when no token is present yet.

## 10. Helper permanently empty — `add_node` `touch` blocks the real install

- **File**: `src/remo_cli/providers/incus.py:706` (and `providers/proxmox.py:128`); blocks `ansible/roles/incus_bootstrap/tasks/main.yml` copy task with `force: false`
- **Failure**: \`add_node\` SSH-runs \`sudo touch /usr/local/libexec/remo-broker-tokens; sudo chmod 0755 ...\`, creating an empty zero-byte file. The later \`incus_bootstrap\` role's helper copy task uses \`force: false\` so it never overwrites. The broker daemon's expected node-side dispatcher exits 0 with no output. Every per-developer token-management on Incus/Proxmox is silently broken.
- **Fix**: Drop the `touch`; let the Ansible role install the real helper. Or change the Ansible task to `force: true` once the helper script content is known.

## 11. Supply-chain — whitespace-only `.sha256` body disables integrity check

- **File**: `ansible/roles/broker_install/tasks/main.yml:44`
- **Failure**: \`failed_when: content == ''\` only catches a literally empty body. A \`.sha256\` containing only \`\\n\` or spaces passes the check; \`(content).split() | first | default('')\` yields \`''\`; \`get_url\` runs with \`checksum: \"sha256:\"\` which Ansible accepts as no-verify. A compromised mirror could substitute a tampered broker binary and the install would succeed silently.
- **Fix**: Validate the extracted checksum against `^[0-9a-fA-F]{64}$` before passing to `get_url`.

## 12. `remo init` is a no-op — config.yml not bridged to env

- **File**: `src/remo_cli/cli/init.py:37` (and every consumer that reads `os.environ.get(\"REMO_BROKER_BACKEND\", \"\")`)
- **Failure**: \`_save_config\` persists \`broker.backend\` to \`~/.config/remo/config.yml\`. But \`providers/hetzner.py:298\`, \`providers/aws.py:561\`, \`core/broker_revoke.py:26\`, \`cli/rotate.py:85\` all read the env var. After \`remo init --backend 1password\` the broker code paths remain inert until the user separately exports \`REMO_BROKER_BACKEND=1password\`.
- **Fix**: Add `core/broker_config.py::get_backend()` with env-first / file-fallback resolution; replace every direct `os.environ.get(\"REMO_BROKER_BACKEND\", ...)` with the helper. Same treatment for `REMO_BROKER_ADMIN_SA_KEY`.

## 13. JSONC strip corrupts string literals containing `//`

- **File**: `src/remo_cli/core/devcontainer.py:59`
- **Failure**: Regex \`(^|[^:])//[^\\n]*\` strips \`//\` line-comments but also matches inside string literals whenever the preceding char isn't \`:\` (e.g. \`\"foo\": \"a//b\"\`). \`json.loads\` then fails; \`ensure_socket_mount\` returns False silently. Currently masked by the helper having no production callers; will bite the moment integration ships.
- **Fix**: Use a proper JSONC tokenizer (or commentjson / json5) that skips quoted strings. Add a regression test for `\"path\": \"a//b\"`.

## 14. Naive vs aware datetime — `--since` filter and freshness check crash uncaught

- **File**: `src/remo_cli/cli/rotate.py:78` (`_parse_iso`) and `src/remo_cli/core/audit.py:100` (`_parse_ts`)
- **Failure**: \`datetime.fromisoformat\` returns a NAIVE datetime when input lacks \`Z\` or explicit offset. \`_now() - last_rotation\` (rotate.py:78 freshness check, :71 `_is_overdue`) and \`_parse_ts(ln.ts) >= cutoff\` (audit.py:68) then raise \`TypeError: can't subtract offset-naive and offset-aware datetimes\`. \`remo rotate-bootstrap\` and \`remo audit --since 1h\` crash uncaught if any broker-written timestamp lacks the offset suffix.
- **Fix**: Normalize to aware in `_parse_iso`/`_parse_ts` (`tzinfo=timezone.utc` if absent). Add tests with bare-ISO inputs.

## 15. SSH MITM window — `accept-new` on a freshly-allocated public IP

- **File**: `src/remo_cli/providers/hetzner.py:132`
- **Failure**: First SSH push uses \`-o StrictHostKeyChecking=accept-new\`. No out-of-band host-key verification. An on-path attacker (BGP hijack, hostile network, stale IP reuse) presenting their own host key gets the bootstrap token piped on stdin; \`accept-new\` pins it without complaint, subsequent runs continue silently against the wrong endpoint.
- **Fix**: Hetzner Cloud's API surfaces the host fingerprint after server creation — fetch it and verify against the live key before the push. Failing that, document the residual risk in \`docs/credential-broker.md\` threat model.

---

**Method**: 5 parallel finder angles (line-by-line / removed-behavior / cross-file / language-pitfalls / wrapper-correctness) → 8 candidates each → dedup → 1 verifier with quote-the-line evidence → sweep for gaps. Of 40 raw candidates, 24 verified CONFIRMED, 2 PLAUSIBLE, 1 REFUTED; 5 from the sweep made the top-15 cut.

cc / context: #32 (PR introducing these), `specs/005-credential-broker/`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Split the collection into per-finding issues, then read the referenced files and the related contracts and tasks under specs/005-credential-broker/. Start with the highest-severity findings and compare each claimed failure with the cited entry point; run the relevant existing tests and add the regression coverage named in the findings. Done means each confirmed defect has a focused change, verification, and an unambiguous result.

Written by the indexing model from the issue text.