orca/.ciagent/REVIEW_v0.8.md

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).