sepehr/Entropyk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Entropyk/_bmad-output/implementation-artifacts/3-4-code-review-findings.md

7.0 KiB
Raw Blame History

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)

  • Story file loaded and parsed
  • Git status/diff compared to story File List
  • Acceptance Criteria checked against implementation
  • Tasks marked [x] verified for actual completion
  • Code quality and architecture compliance reviewed
  • Test coverage vs story requirements checked
  • 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.