|
|
|
@@ -0,0 +1,321 @@
|
|
|
|
|
# Review: Orca v0.8 — Coverage & Trust Hardening (final-phase)
|
|
|
|
|
|
|
|
|
|
**Reviewer**: ci-code-reviewer (multi-persona: correctness, testing, security, performance, maintainability, adversarial)
|
|
|
|
|
**Branch**: `phase/04-final-review-ship` (review HEAD = P03 ship `70c5718`)
|
|
|
|
|
**Diff scope**: `main...milestone/v0.8-coverage-trust-hardening` (all v0.8 work, 59 files, +6550/-173)
|
|
|
|
|
**Date**: 2026-08-04
|
|
|
|
|
**Verdict**: **PASS-WITH-FOLLOWUPS** (0 P0, 2 P1, 2 P2)
|
|
|
|
|
|
|
|
|
|
## Methodology
|
|
|
|
|
|
|
|
|
|
Read the full diff (`internal/`, `cmd/`, `Makefile`, `.coreci.yml`), all 3 phase
|
|
|
|
|
verification reports, PLAN/RESEARCH/GRILL/PERSONAS, and the critical production
|
|
|
|
|
files directly (`internal/proxmox/bootstrap.go`, `internal/security/ca.go`,
|
|
|
|
|
`internal/security/sshkey.go`, `internal/doctor/doctor.go:400-454`,
|
|
|
|
|
`cmd/verify-reqs/main.go`). Re-ran `go build ./...`, `go vet ./...`,
|
|
|
|
|
`go test -race` on proxmox/security/doctor/cli/verify-reqs/cmd-orca, and
|
|
|
|
|
`make verify-reqs` (all PASS). Re-verified the verify-reqs regex against the
|
|
|
|
|
real ROADMAP.md (matches v0.1..v0.7 COMPLETE incl. v0.2 parenthetical; v0.8
|
|
|
|
|
correctly not matched). Confirmed all 7 T02.10 e2e cases are present and
|
|
|
|
|
exercised through a real in-process SSH server.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Per-Axis Findings
|
|
|
|
|
|
|
|
|
|
### 1. Correctness (lead-developer)
|
|
|
|
|
|
|
|
|
|
**C1 — `sessionRunner` seam backward-compat** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:170-172,322-339`. The seam is a package-level
|
|
|
|
|
`var sessionRunner sessionRunnerType` (line 326) initialized lazily inside
|
|
|
|
|
`BootstrapProxmox` from the dialed `*ssh.Client` (`if sessionRunner == nil {
|
|
|
|
|
sessionRunner = &sshSessionRunner{client: conn} }`). Existing callers are
|
|
|
|
|
unchanged — the default `sshSessionRunner` wraps the real
|
|
|
|
|
`conn.NewSession().CombinedOutput(...)`. Tests reset `sessionRunner = nil`
|
|
|
|
|
between runs (bootstrap_test.go:910, 1010, 1035) to avoid cross-test leakage.
|
|
|
|
|
Backward compatible as required by P01 verification. No issue.
|
|
|
|
|
|
|
|
|
|
**C2 — `verify-reqs` parser correctness** ✅
|
|
|
|
|
`cmd/verify-reqs/main.go:18-26`. The `reqRowRe` uses a greedy `.*` for the
|
|
|
|
|
Requirement+Priority cells and anchors the Phase+Status match at the END of
|
|
|
|
|
the line, where those two columns always live. This correctly handles
|
|
|
|
|
escaped pipes inside the Requirement cell (e.g. REQ-049
|
|
|
|
|
`localhost\|linux\|proxmox` — verified by the passing `make verify-reqs`
|
|
|
|
|
which reports 60 consistent rows, matching the 60 REQ rows in
|
|
|
|
|
REQUIREMENTS.md). The status token is optionally bold-wrapped
|
|
|
|
|
(`\*{0,2}(Complete|Pending)\*{0,2}`) with `[^|]*` for trailing notes —
|
|
|
|
|
handles `**Complete** (P01 shipped v0.2.1)`. The milestone-complete regex
|
|
|
|
|
`^##\s*Milestone\s+(v0\.\d+):.*—\s*\*\*[^*]*\bCOMPLETE\b[^*]*\*\*` is
|
|
|
|
|
substring-tolerant (GRILL #4) — verified against the real ROADMAP: matches
|
|
|
|
|
v0.1..v0.7 incl. v0.2's `**COMPLETE (merged to main via v0.3)**` and v0.6's
|
|
|
|
|
duplicate header (line 94 matched; line 92 without COMPLETE ignored). v0.8
|
|
|
|
|
(line 136, not yet COMPLETE) correctly not matched — so the v0.8 REQ rows
|
|
|
|
|
being `Pending` is NOT flagged as drift (correct: milestone not shipped
|
|
|
|
|
yet). No issue.
|
|
|
|
|
|
|
|
|
|
**C3 — TOFU capture-fix logic** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:275-310`. On `*knownhosts.KeyError` with
|
|
|
|
|
empty `Want` (host unknown), captures the key, reads existing known_hosts
|
|
|
|
|
(create-if-missing), ensures trailing newline, appends
|
|
|
|
|
`knownhosts.Line([]string{knownhosts.Normalize(addr)}, key)`, writes via
|
|
|
|
|
`security.WriteAtomic`, returns nil (dial proceeds). On non-empty `Want`
|
|
|
|
|
(mismatch) returns the error (MITM detection preserved). On `nil` (match)
|
|
|
|
|
records key + returns nil. Correct against x/crypto v0.54.0 `checkAddr`
|
|
|
|
|
semantics. No issue.
|
|
|
|
|
|
|
|
|
|
**C4 — `ResetHostKey` line matching** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:479-523`. Matches a line when its first
|
|
|
|
|
whitespace-delimited field, normalized via `knownhosts.Normalize`, equals
|
|
|
|
|
the normalized target. Handles `[host]:22` vs bare `host` (Normalize
|
|
|
|
|
brackets ports). Preserves comments/blanks. Atomic rewrite via
|
|
|
|
|
`security.WriteAtomic`. Edge cases handled: empty host errors, missing
|
|
|
|
|
file is a no-op, no matching lines is a no-op. Test
|
|
|
|
|
`TestResetHostKey_RemovesTargetLines` (bootstrap_test.go:718) seeds 2 lines
|
|
|
|
|
for target + 1 for another host, asserts target's 2 removed + other
|
|
|
|
|
intact. No issue.
|
|
|
|
|
|
|
|
|
|
**C5 — `--host-key-fingerprint` non-proxmox validation** ✅
|
|
|
|
|
`internal/cli/node.go:79-81`. `RunE` checks `joinHostKeyFP != "" &&
|
|
|
|
|
joinType != "proxmox"` → clear error. Test
|
|
|
|
|
`TestNodeJoinHostKeyFingerprintRequiresProxmox` (node_test.go:445) asserts
|
|
|
|
|
the error; `TestNodeJoinHostKeyFingerprintProxmoxAccepted` (node_test.go:477)
|
|
|
|
|
asserts the negative-space (proxmox type accepts the flag). No issue.
|
|
|
|
|
|
|
|
|
|
### 2. Testing (all personas)
|
|
|
|
|
|
|
|
|
|
**T1 — Coverage held post-P02** ✅
|
|
|
|
|
PHASE2 verification: proxmox 86.5% (was 87.1%), cli 76.7% (was 76.2%),
|
|
|
|
|
doctor 70.4%. Marginal changes from new code paths — no coverage regression.
|
|
|
|
|
Re-ran `go test -race ./internal/proxmox/... ./internal/cli/...` PASS.
|
|
|
|
|
|
|
|
|
|
**T2 — Race tests pass** ✅
|
|
|
|
|
`go test -race -count=1 ./internal/proxmox/... ./internal/security/...
|
|
|
|
|
./internal/doctor/... ./cmd/verify-reqs/... ./cmd/orca/...` all PASS.
|
|
|
|
|
`./internal/cli/...` PASS (77s, dominated by watch tests). No races.
|
|
|
|
|
|
|
|
|
|
**T3 — 7 T02.10 integration cases** ✅
|
|
|
|
|
All 7 present in `internal/proxmox/bootstrap_test.go`, exercised
|
|
|
|
|
end-to-end through `BootstrapProxmox` with a real in-process SSH server
|
|
|
|
|
(`bootstrapE2ESetup`):
|
|
|
|
|
- Case 1: `TestBootstrapE2E_PinnedFingerprintCorrect` (815)
|
|
|
|
|
- Case 2: `TestBootstrapE2E_PinnedFingerprintWrong` (842)
|
|
|
|
|
- Case 3: `TestBootstrapE2E_TOFUFirstConnectCapturesKey` (865)
|
|
|
|
|
- Case 4: `TestBootstrapE2E_TOFUSecondConnectMatches` (905)
|
|
|
|
|
- Case 5: `TestBootstrapE2E_TOFUMismatchFails` (925)
|
|
|
|
|
- Case 6: `TestBootstrapE2E_KeyResetThenRePin` (1002)
|
|
|
|
|
- Case 7: `TestBootstrapE2E_PrePopulatedKnownHostsMatches` (966, v0.6→v0.8 migration)
|
|
|
|
|
|
|
|
|
|
**T4 — Golden tests for verify-reqs** ✅
|
|
|
|
|
`cmd/verify-reqs/main_test.go` has 7 tests covering: clean pair, multi-drift
|
|
|
|
|
(both directions), default-args subprocess, malformed (no REQ rows → error),
|
|
|
|
|
missing file → error, v0.2 substring-tolerant header regression guard
|
|
|
|
|
(`TestVerify_v02SubstringTolerantHeader`), and real-repo regression guard.
|
|
|
|
|
The substring-tolerant regex is explicitly exercised — the drift fixture's
|
|
|
|
|
ROADMAP uses `**COMPLETE (merged to main via v0.3)**` on v0.2 and the test
|
|
|
|
|
asserts REQ-002 (v0.2 P1, Pending) is flagged forward-drift (would be
|
|
|
|
|
silently skipped if the regex regressed). No issue.
|
|
|
|
|
|
|
|
|
|
**T5 — Doctor parity test** ✅
|
|
|
|
|
`internal/doctor/doctor_test.go` extended with 94 LOC covering
|
|
|
|
|
`probeProxmoxPVEVersion` paths. The doctor callback now uses the shared
|
|
|
|
|
`proxmox.TOFUHostKeyCallback` (doctor.go:424) — GRILL #2 parity verified by
|
|
|
|
|
reading both call sites. No issue.
|
|
|
|
|
|
|
|
|
|
### 3. Security (backend-engineer)
|
|
|
|
|
|
|
|
|
|
**S1 — `--host-key-fingerprint` fails closed** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:141-153,245-258`. The pinned/TOFU branch is
|
|
|
|
|
mutually exclusive (`if opts.HostKeyFingerprint != "" { ... } else { ... }`).
|
|
|
|
|
The pinned callback (245-258) validates `SHA256:` prefix up front (rejects
|
|
|
|
|
raw hex per D-045), computes `ssh.FingerprintSHA256(key)`, returns an error
|
|
|
|
|
on any mismatch — no fallback to TOFU. The dial (164) aborts on callback
|
|
|
|
|
error before any SSH session command runs. Cannot be bypassed: the pin is
|
|
|
|
|
compared as a full string against the canonical fingerprint of the
|
|
|
|
|
server-presented key; a mismatch returns before `*capturedKey` is set. No
|
|
|
|
|
issue.
|
|
|
|
|
|
|
|
|
|
**S2 — `key-reset` is local-only (D-046)** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:479-523` + `internal/cli/node.go:348-407`.
|
|
|
|
|
`ResetHostKey` only reads/writes `certpaths.KnownHostsPath()`. No SSH dial,
|
|
|
|
|
no remote authorized_keys touch. Audit-logs `node.key_reset` with
|
|
|
|
|
actor+node+host (node.go:396-400). Verified by `TestNodeKeyReset`
|
|
|
|
|
(node_test.go:326) which asserts the audit row. No issue.
|
|
|
|
|
|
|
|
|
|
**S3 — TOFU capture-fix doesn't weaken MITM detection** ✅
|
|
|
|
|
See C3 — the fix ONLY captures on `KeyError{Want:[]}` (host unknown); a
|
|
|
|
|
non-empty `Want` (key mismatch / MITM) returns the error. The capture path
|
|
|
|
|
writes the server-presented key, so a subsequent different key fails. No
|
|
|
|
|
issue.
|
|
|
|
|
|
|
|
|
|
**S4 — `WriteAtomic` is actually atomic** ✅
|
|
|
|
|
`internal/security/ca.go:308-337`. Temp file in same dir
|
|
|
|
|
(`os.CreateTemp(dir, ".tmp-*")`), `Write`, `Chmod`, `Sync`, `Close`, then
|
|
|
|
|
`os.Rename` (atomic on POSIX same-filesystem). `defer os.Remove(tmpName)`
|
|
|
|
|
cleans up on failure. Genuine atomic-write pattern. No issue.
|
|
|
|
|
|
|
|
|
|
### 4. Performance (all)
|
|
|
|
|
|
|
|
|
|
**P1 — ResetHostKey is O(n) in file size** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:479-523`: one `os.ReadFile` (O(n)), one
|
|
|
|
|
`strings.Split` + linear filter loop (O(n)), one `security.WriteAtomic`
|
|
|
|
|
(O(n)). No nested loops, no O(n²). For a known_hosts file (typically tens
|
|
|
|
|
of lines), this is negligible. No issue.
|
|
|
|
|
|
|
|
|
|
**P2 — Unnecessary allocations** (P2 — nit)
|
|
|
|
|
`bootstrap.go:494-510`: `strings.Split(string(existing), "\n")` allocates a
|
|
|
|
|
slice of all lines + `append(kept, []byte(line+"\n")...)` reallocates the
|
|
|
|
|
kept buffer. For known_hosts (small file) this is fine; a `bufio.Scanner`
|
|
|
|
|
over `bytes.NewReader(existing)` with a `strings.Builder` would be leaner,
|
|
|
|
|
but the current shape is clear and the file is tiny. Not worth changing.
|
|
|
|
|
Flagged P2 (nit, no action).
|
|
|
|
|
|
|
|
|
|
### 5. Maintainability (lead-developer)
|
|
|
|
|
|
|
|
|
|
**M1 — `TOFUHostKeyCallback` extraction** ✅
|
|
|
|
|
`internal/proxmox/bootstrap.go:261-310` is exported and shared by bootstrap
|
|
|
|
|
(148) and doctor (doctor.go:424) via
|
|
|
|
|
`proxmox.TOFUHostKeyCallback(sshAddr, &capturedHostKey)`. Clean coupling:
|
|
|
|
|
doctor imports proxmox (one-way), no duplication, no circular dep. The
|
|
|
|
|
GRILL #2 parity requirement (both call sites use the same wrapper) is
|
|
|
|
|
satisfied by construction. No issue.
|
|
|
|
|
|
|
|
|
|
**M2 — `verify()` testable** ✅
|
|
|
|
|
`cmd/verify-reqs/main.go:98-148`: the core logic is a pure function
|
|
|
|
|
`verify(roadmapPath, reqsPath string) (diff []string, count int, err error)`
|
|
|
|
|
with `main()` as a thin wrapper. Golden-file tests call `verify()` directly
|
|
|
|
|
(no subprocess). Mirrors the T01.11 `main()→run()` pattern. No issue.
|
|
|
|
|
|
|
|
|
|
**M3 — cli tests follow conventions** ✅
|
|
|
|
|
`internal/cli/namespace_test.go:21-35` adds `resetCommandFlags()` to zero
|
|
|
|
|
the package-level flag-bound vars between subtests (cobra parses into
|
|
|
|
|
globals; without reset a prior test's value persists). Called from
|
|
|
|
|
`resetRootFlags`. This is a sound convention — the test isolation is
|
|
|
|
|
correct. No issue.
|
|
|
|
|
|
|
|
|
|
**M4 — `resetCommandFlags` completeness** (P2 — nit)
|
|
|
|
|
`namespace_test.go:28-34` resets the join/leave/cap/audit/run/stop flags but
|
|
|
|
|
NOT `joinHostKeyFP`. A test that sets `--host-key-fingerprint` without
|
|
|
|
|
calling `resetRootFlags` could leak the value to a later test. In practice
|
|
|
|
|
all node tests call `resetRootFlags` which calls `resetCommandFlags`, so
|
|
|
|
|
this is a latent risk only. Recommend adding `joinHostKeyFP = ""` to
|
|
|
|
|
`resetCommandFlags` for completeness. Flagged P2 (nit).
|
|
|
|
|
|
|
|
|
|
### 6. Adversarial (backend-engineer)
|
|
|
|
|
|
|
|
|
|
**A1 — Can `--host-key-fingerprint` be bypassed?** ✅
|
|
|
|
|
No. The pinned callback (bootstrap.go:249-258) returns an error before
|
|
|
|
|
recording the key or allowing the dial to proceed on any mismatch. There is
|
|
|
|
|
no code path where a supplied pin is ignored — the branch at 141-153 is
|
|
|
|
|
`if opts.HostKeyFingerprint != ""` (pinned) `else` (TOFU); once pinned is
|
|
|
|
|
chosen, TOFU is not consulted. No bypass.
|
|
|
|
|
|
|
|
|
|
**A2 — Can `key-reset` corrupt known_hosts under concurrent write?** (P1 — important, low likelihood)
|
|
|
|
|
`proxmox.ResetHostKey` (bootstrap.go:479-523) and `TOFUHostKeyCallback`
|
|
|
|
|
(bootstrap.go:290-302) both do read-modify-write on
|
|
|
|
|
`certpaths.KnownHostsPath()` WITHOUT a lock. Two concurrent operations
|
|
|
|
|
(e.g. `orca node join --type proxmox hostA` + `orca node key-reset hostB`,
|
|
|
|
|
or two simultaneous joins to different hosts) could interleave:
|
|
|
|
|
- T1 reads known_hosts (empty), T2 reads known_hosts (empty)
|
|
|
|
|
- T1 writes hostA line, T2 writes hostB line
|
|
|
|
|
- Last rename wins → one line lost.
|
|
|
|
|
|
|
|
|
|
The `security.WriteAtomic` (temp+rename) prevents corruption (the file is
|
|
|
|
|
always valid OpenSSH format), but a captured line can be silently lost. This
|
|
|
|
|
is a **last-writer-wins race on a flat file with no lock**. Severity is low
|
|
|
|
|
because orca is a single-operator CLI (concurrent joins are unusual) and
|
|
|
|
|
the lost line is recoverable (re-connect re-pins via TOFU). But it is a
|
|
|
|
|
real correctness gap for the trust surface. Recommend either (a) a
|
|
|
|
|
file-lock around the read-modify-write, or (b) documenting the
|
|
|
|
|
single-operator assumption explicitly. Flagged P1 (important, post-hoc).
|
|
|
|
|
|
|
|
|
|
**A3 — Can verify-reqs be fooled by a crafted markdown table?** ✅
|
|
|
|
|
No. The `reqRowRe` anchors on `^\|\s*(REQ-\d+)\s*\|` and the status column
|
|
|
|
|
at end-of-line. A crafted row with a fake status would have to match the
|
|
|
|
|
regex exactly. The "malformed" fixture (`requirements_malformed.md`)
|
|
|
|
|
exercises the no-REQ-rows path → clear error. A row like
|
|
|
|
|
`| REQ-999 | missing status cell | High | v0.1 |` (no final `|...|`) does
|
|
|
|
|
NOT match `reqRowRe` (the trailing `\|\s*$` requires the status cell) — it
|
|
|
|
|
is silently skipped, which `verify` reports as "no REQ rows" only if ALL
|
|
|
|
|
rows are malformed. If some rows are valid + one malformed, the malformed
|
|
|
|
|
row is skipped without error — a minor blind spot, but acceptable (the
|
|
|
|
|
gate catches drift, not typos). No blocking issue.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## GRILL Conditions Verification
|
|
|
|
|
|
|
|
|
|
### #1 — T02.6 labeled as v0.6 ship-defect bugfix ✅
|
|
|
|
|
Commit `8b0cbe1` summary: "fix(proxmox): TOFU capture bug — **v0.6
|
|
|
|
|
ship-defect** first-connect join always failed (T02.6)". The commit message
|
|
|
|
|
explicitly labels it as a v0.6 ship-defect bugfix, not a v0.8 feature.
|
|
|
|
|
PHASE2 verification report records it as "TOFU bugfix (T02.6, v0.6
|
|
|
|
|
ship-defect)". **Satisfied.**
|
|
|
|
|
|
|
|
|
|
### #2 — T02.9 doctor parity (bootstrap + doctor use capture-fix wrapper) ✅
|
|
|
|
|
- Bootstrap: `internal/proxmox/bootstrap.go:148` calls
|
|
|
|
|
`TOFUHostKeyCallback(sshAddr, &capturedHostKey)`.
|
|
|
|
|
- Doctor: `internal/doctor/doctor.go:424` calls
|
|
|
|
|
`proxmox.TOFUHostKeyCallback(sshAddr, nil)`.
|
|
|
|
|
Both use the SAME exported wrapper (`proxmox.TOFUHostKeyCallback`,
|
|
|
|
|
bootstrap.go:275). No duplication. The doctor diff
|
|
|
|
|
(`internal/doctor/doctor.go`) removes the direct `knownhosts.New` call and
|
|
|
|
|
replaces it with the shared wrapper. **Satisfied.**
|
|
|
|
|
|
|
|
|
|
### #3 — T01.6 cli escape valve (was it needed?) ✅
|
|
|
|
|
PHASE1 verification: cli hit **76.2%** (above the 70% floor, excluding
|
|
|
|
|
daemon.go). The escape valve (ship at 65% if 70% not reached) was **NOT
|
|
|
|
|
needed**. The plan's conditional was correctly conservative; the actual
|
|
|
|
|
result exceeded the floor. **Satisfied (not invoked).**
|
|
|
|
|
|
|
|
|
|
### #4 — T03.1 verify-reqs regex substring-tolerant + reverse direction ✅
|
|
|
|
|
- **Substring-tolerant**: `cmd/verify-reqs/main.go:30`:
|
|
|
|
|
`^##\s*Milestone\s+(v0\.\d+):.*—\s*\*\*[^*]*\bCOMPLETE\b[^*]*\*\*`.
|
|
|
|
|
Verified against the real ROADMAP: matches v0.2's
|
|
|
|
|
`**COMPLETE (merged to main via v0.3)**` and all other COMPLETE
|
|
|
|
|
milestones. Golden test `TestVerify_v02SubstringTolerantHeader`
|
|
|
|
|
(main_test.go:140) guards against regression.
|
|
|
|
|
- **Reverse direction**: `cmd/verify-reqs/main.go:135-139`: a REQ marked
|
|
|
|
|
`Complete` whose referenced milestones are ALL not-C_COMPLETE in ROADMAP
|
|
|
|
|
is flagged as `direction=reverse` drift. Golden test `TestVerify_drift`
|
|
|
|
|
asserts REQ-003 is reverse-drift.
|
|
|
|
|
- **Scope note**: PLAN + commit `fc2b020` document that REQ-060 catches
|
|
|
|
|
doc-vs-doc drift only (code-vs-doc like the REQ-053 cert_repo_test.go
|
|
|
|
|
case is out of scope; P04 audit is the backstop). **Satisfied.**
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## P0 Fixes Applied
|
|
|
|
|
|
|
|
|
|
**None.** No P0 issues (correctness bugs, security holes, broken build/test)
|
|
|
|
|
were found. `go build ./...`, `go vet ./...`, `go test -race` (all key
|
|
|
|
|
packages), and `make verify-reqs` all PASS. The milestone is shippable as-is.
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## P1+ Issues Flagged (post-hoc review)
|
|
|
|
|
|
|
|
|
|
| ID | Severity | File:line | Issue | Recommendation |
|
|
|
|
|
|----|----------|-----------|-------|----------------|
|
|
|
|
|
| A2 | P1 (important, low likelihood) | `internal/proxmox/bootstrap.go:290-302, 479-523` | `TOFUHostKeyCallback` capture path and `ResetHostKey` both do read-modify-write on `known_hosts` with no lock; concurrent operations can lose a captured line (last-writer-wins via atomic rename — no corruption, but data loss). | Add a file-lock (`flock` on a `.known_hosts.lock` sibling, or `github.com/gofrs/flock` if a dep is acceptable) around the RMW in both paths; OR document the single-operator assumption in the key-reset help text. Defer to v0.9. |
|
|
|
|
|
| M4 | P2 (nit) | `internal/cli/namespace_test.go:28-34` | `resetCommandFlags()` does not reset `joinHostKeyFP`; a test setting `--host-key-fingerprint` without `resetRootFlags` could leak the value. | Add `joinHostKeyFP = ""` to `resetCommandFlags`. Trivial. |
|
|
|
|
|
| P2 | P2 (nit) | `internal/proxmox/bootstrap.go:494-510` | `ResetHostKey` uses `strings.Split` + repeated `append` (minor allocation churn). | Optional: use `bufio.Scanner` + `strings.Builder`. Not worth changing for a small file. |
|
|
|
|
|
|
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
## Overall Verdict
|
|
|
|
|
|
|
|
|
|
**PASS-WITH-FOLLOWUPS**
|
|
|
|
|
|
|
|
|
|
The v0.8 milestone is correct, secure, tested, and shippable. All 4 GRILL
|
|
|
|
|
binding conditions are satisfied. Zero P0 issues. The single P1 (concurrent
|
|
|
|
|
`known_hosts` write race, A2) is a real but low-likelihood gap appropriate
|
|
|
|
|
for post-hoc follow-up — it does not block the milestone ship because orca
|
|
|
|
|
is a single-operator CLI and the atomic-rename guarantees the file is never
|
|
|
|
|
corrupted (only a captured line can be lost, recoverable on re-connect).
|
|
|
|
|
The 2 P2 nits are cosmetic. Coverage held post-P02 (86.5%/76.7%/70.4% for
|
|
|
|
|
proxmox/cli/doctor), race tests pass, all 7 T02.10 e2e cases are present and
|
|
|
|
|
exercised through a real in-process SSH server, and `make verify-reqs`
|
|
|
|
|
passes on the current repo (60 consistent rows).
|
|
|
|
|
|
|
|
|
|
Recommend proceeding to P04 ship (T04.7/T04.8: mark REQ-057..060 Complete +
|
|
|
|
|
ROADMAP v0.8 COMPLETE, then tag v0.7.4).
|
Jon Chery