# Code Review Findings — Story 3.4: Thermal Coupling Between Circuits

**Story:** 3-4-thermal-coupling-between-circuits.md  
**Reviewer:** Adversarial Senior Developer (workflow)  
**Date:** 2026-02-17  
**Git vs Story Discrepancies:** 3 (files modified/untracked not in File List; solver crate untracked)

---

## Summary

| Severity | Count |
|----------|--------|
| CRITICAL | 1 |
| HIGH | 1 |
| MEDIUM | 4 |
| LOW | 2 |

**Total issues:** 8

---

## CRITICAL ISSUES

### C1. Task marked [x] but not implemented — coupling residuals and Jacobian (AC#4)

**Location:** Story Tasks "Expose coupling residuals for solver (AC: #4)" — marked complete.

**Evidence:**  
- Story requires: `coupling_residuals(state: &SystemState) -> Vec<f64>` and `coupling_jacobian_entries()` returning `(row, col, partial_derivative)` tuples.
- **Neither API exists** in `crates/solver/src/system.rs` or `crates/solver/src/coupling.rs`.
- AC#4 states: "When solver assembles residuals, coupling contributes additional residual equations" and "Jacobian includes coupling derivatives". The solver has no way to obtain coupling residuals or Jacobian entries.

**Impact:** Story 4.x (Solver) cannot consume thermal coupling in the solve loop. AC#4 is **not implemented**.

**Recommendation:** Implement `coupling_residuals` and `coupling_jacobian_entries` (or equivalent) on `System` or in a solver-facing module, or mark the task and AC#4 as partial and add follow-up tasks.

---

## HIGH ISSUES

### H1. Acceptance Criterion #4 not implemented

**Criterion:** "Given a defined thermal coupling, when solver assembles residuals, then coupling contributes additional residual equations and Jacobian includes coupling derivatives."

**Evidence:** No function in the codebase computes coupling residuals from `SystemState` or provides Jacobian entries for couplings. The story is marked as satisfying all ACs; AC#4 is **MISSING**.

**Recommendation:** Either implement the residual and Jacobian coupling API or update the story to reflect partial completion and add a follow-up story/task.

---

## MEDIUM ISSUES

### M1. Missing tracing::warn for circular dependency (Architecture Compliance)

**Location:** `crates/solver/src/coupling.rs` and `finalize()` path.

**Evidence:**  
- Architecture Compliance in story: "tracing for circular dependency warnings: `tracing::warn!(\"Circular thermal coupling detected, simultaneous solving required\")`".
- `has_circular_dependencies()` is implemented but **never logs**. No call to `tracing::warn!` when circular dependencies are detected (e.g. in `finalize()` or when adding a coupling that creates a cycle).

**Recommendation:** In `System::finalize()`, after topology validation, call `has_circular_dependencies(self.thermal_couplings())` and if true, emit `tracing::warn!(...)` as specified.

---

### M2. Files changed but not in story File List

**Evidence (git vs story):**  
- **In git** (modified or untracked) but **not listed** in Dev Agent Record → File List:  
  - `Cargo.toml` (workspace root)  
  - `_bmad-output/implementation-artifacts/sprint-status.yaml`  
  - `_bmad-output/planning-artifacts/epics.md`  
  - `_bmad-output/planning-artifacts/prd.md`  
- Entire `crates/solver/` is untracked (??); story File List correctly lists solver files as new/modified, but sprint-status and planning-artifacts changes are not documented.

**Recommendation:** Either add the modified planning/artifacts files to the File List (if they were intentionally changed for this story) or note in Change Log that only code changes are claimed for 3.4.

---

### M3. Integration test "test_coupling_residuals_basic" missing

**Location:** Story Testing Requirements — "Integration tests (system.rs or multi_circuit.rs): test_coupling_residuals_basic — 2 circuits with coupling, verify residual equation."

**Evidence:**  
- `crates/solver/tests/multi_circuit.rs` exists and has tests for multi-circuit topology and thermal coupling add/get, but **no test** that verifies "residual equation" for couplings.
- Such a test cannot be fully implemented until `coupling_residuals` exists; the story nonetheless claims the task "Tests" as done and lists this test.

**Recommendation:** Add a placeholder or skip test that documents the requirement, and implement the real test when `coupling_residuals` is added.

---

### M4. finalize() does not run circular-dependency check or log

**Location:** Story Dev Notes — "Circular dependency detection at finalize time"; Architecture — "Coupling graph built once at finalize/build time."

**Evidence:**  
- `System::finalize()` calls `validate_topology()` only. It does not call `has_circular_dependencies(&self.thermal_couplings)` nor log that simultaneous solving will be required.
- Solver has no built-time signal that coupling groups / circular dependencies were detected.

**Recommendation:** In `finalize()`, after successful topology validation, if `!self.thermal_couplings.is_empty()` and `has_circular_dependencies(self.thermal_couplings())`, call `tracing::warn!("Circular thermal coupling detected, simultaneous solving required")` and optionally store a flag or coupling groups for the solver to use.

---

## LOW ISSUES

### L1. compute_coupling_heat returns f64 instead of NewType HeatTransfer

**Location:** `crates/solver/src/coupling.rs` — `compute_coupling_heat()` returns `f64`.

**Evidence:**  
- Story Technical Requirements: "Returns HeatTransfer" and "HeatTransfer(pub f64)" in code block; Architecture: "Use ThermalConductance(pub f64), HeatTransfer(pub f64) for clarity" and "never bare f64 in public APIs."
- `entropyk_core` does not define `HeatTransfer`; the function returns raw `f64`. Semantics are correct; only the type-safety and documentation alignment are missing.

**Recommendation:** Add `HeatTransfer(pub f64)` to `crates/core/src/types.rs` (with Display/From/helpers) and change `compute_coupling_heat` to return `HeatTransfer`. Re-export from core and use in solver.

---

### L2. NFR4 / no-allocation in solver loop vs Vec return

**Location:** Story and Architecture.

**Evidence:**  
- Architecture: "No dynamic allocation in solver loop"; Story Dev Notes: "coupling_residuals() called in solver loop — must be fast (no allocation)."
- Story task proposes `coupling_residuals(state: &SystemState) -> Vec<f64>`, which allocates every call.

**Recommendation:** When implementing `coupling_residuals`, prefer a signature that writes into a pre-allocated slice (e.g. `fn coupling_residuals(&self, state: &SystemState, out: &mut [f64])`) so the solver loop can reuse a single buffer and respect NFR4.

---

## Checklist (validation)

- [x] Story file loaded and parsed
- [x] Git status/diff compared to story File List
- [x] Acceptance Criteria checked against implementation
- [x] Tasks marked [x] verified for actual completion
- [x] Code quality and architecture compliance reviewed
- [x] Test coverage vs story requirements checked
- [x] Outcome: **Changes applied** (CRITICAL/HIGH/MEDIUM fixes applied 2026-02-17; story status → done)

---

*Review completed. CRITICAL and HIGH issues fixed automatically; story status set to done.*