azcomputerguru/guru-connect
1
0
Fork 0
Code Issues 18 Pull Requests Actions Packages Projects Releases 2 Wiki Activity

SPEC-018 review: deferred hardening follow-ups (hot-path unwraps, panic-guard scope, nits) #8

New Issue
Open
opened 2026-06-03 15:13:55 -07:00 by azcomputerguru · 0 comments
azcomputerguru commented 2026-06-03 15:13:55 -07:00
Owner
Copy Link

Follow-up items from the SPEC-018 Phase 1 code review (reports/2026-06-03-spec018-review.md). The two real bugs (Issue 1 non-functional managed fallback, Issue 2 agent_id churn) and Issue 6 (registry HANDLE→HKEY transmute) are being fixed on branch fix/spec018-review-bugs. These remaining items are lower-severity and deferred:

  • Issue 3 (suggestion) — agent/src/session/mod.rs:386, 486, 559: self.transport.as_mut().unwrap() in the connected hot loop (recv / chat poll / frame send). Currently safe by control flow but violates the no-unwrap-in-hot-path rule on the SYSTEM service surface. Replace with expect("…") or if let Some(t) = … else break guards; audit similar patterns in the connected loop.
  • Issue 4 (suggestion) — agent/src/main.rs:442-466 / service/mod.rs:103-167: the catch_unwind panic guard only wraps the inner agent runtime (rt.block_on), not the thin status/reporting code in run_service() itself. Consider wrapping more of run_service (or guarding inside service_main) so the "panic across SCM FFI" guarantee is complete.
  • Issue 5 (suggestion) — agent/src/service/mod.rs:353, 470-476: blocking std::thread::sleep in create_service_with_retry and the 10×500ms stop_if_running poll. Install-time only (not in the SCM handler), acceptable; document as install-time-only or convert if ever moved under tokio.
  • Issue 7 (nit) — agent/src/transport/websocket.rs:208: remove the now-stale #[allow(dead_code)] on close() (it is live in the SPEC-018 service-stop path).
  • Issue 8 (nit) — agent/src/enroll.rs:329,359,360,373,380 and scattered credential_store.rs test code: .unwrap() in #[cfg(test)]. Convert to ?/expect("test precondition") for hygiene. No runtime impact.

Reference: full review in reports/2026-06-03-spec018-review.md. Independent validation confirmed Issues 1, 2, and 6 as the actionable ones.

Follow-up items from the SPEC-018 Phase 1 code review (`reports/2026-06-03-spec018-review.md`). The two real bugs (Issue 1 non-functional managed fallback, Issue 2 agent_id churn) and Issue 6 (registry HANDLE→HKEY transmute) are being fixed on branch `fix/spec018-review-bugs`. These remaining items are lower-severity and deferred: - **Issue 3 (suggestion)** — `agent/src/session/mod.rs:386, 486, 559`: `self.transport.as_mut().unwrap()` in the connected hot loop (recv / chat poll / frame send). Currently safe by control flow but violates the no-unwrap-in-hot-path rule on the SYSTEM service surface. Replace with `expect("…")` or `if let Some(t) = … else break` guards; audit similar patterns in the connected loop. - **Issue 4 (suggestion)** — `agent/src/main.rs:442-466` / `service/mod.rs:103-167`: the `catch_unwind` panic guard only wraps the inner agent runtime (`rt.block_on`), not the thin status/reporting code in `run_service()` itself. Consider wrapping more of `run_service` (or guarding inside `service_main`) so the "panic across SCM FFI" guarantee is complete. - **Issue 5 (suggestion)** — `agent/src/service/mod.rs:353, 470-476`: blocking `std::thread::sleep` in `create_service_with_retry` and the 10×500ms `stop_if_running` poll. Install-time only (not in the SCM handler), acceptable; document as install-time-only or convert if ever moved under tokio. - **Issue 7 (nit)** — `agent/src/transport/websocket.rs:208`: remove the now-stale `#[allow(dead_code)]` on `close()` (it is live in the SPEC-018 service-stop path). - **Issue 8 (nit)** — `agent/src/enroll.rs:329,359,360,373,380` and scattered `credential_store.rs` test code: `.unwrap()` in `#[cfg(test)]`. Convert to `?`/`expect("test precondition")` for hygiene. No runtime impact. Reference: full review in `reports/2026-06-03-spec018-review.md`. Independent validation confirmed Issues 1, 2, and 6 as the actionable ones.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
component:agent
component:dashboard
component:server
security
severity:critical
severity:high
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
azcomputerguru
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: azcomputerguru/guru-connect#8
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?