code-review

name	code-review
description	End-of-session code review checking for hardcoded values, core vs platform-specific placement, board JSON quality, and bugs in outstanding changes.
allowed-tools	Bash Read Grep Glob Agent

End-of-Session Code Review

You are performing a code review of all changes made during this session. Review the diff of all uncommitted changes and recent commits on this branch.

How to gather changes

# Uncommitted changes (staged + unstaged)
git diff HEAD

# If the working tree is clean, review the most recent commits on this branch
git log --oneline -10
git diff HEAD~5..HEAD  # adjust range as needed

Review every changed file. For each file, apply ALL checks below.

---

Check 1: No Hardcoded Values in Rust Source

Rule: Configuration values, magic numbers, board-specific data, and hardware constants MUST live in JSON data files — NOT as literals in .rs source code. This project has a two-tier config system: board JSONs (crates/fbuild-config/assets/boards/json/) and embedded MCU config JSONs (crates/fbuild-build/src/{platform}/configs/). Use them.

Historical bugs this catches (commits 17ef7cd, 7a8c7ba):

• Teensy defines were hardcoded in Rust instead of teensy31.json — moved to JSON defines array
• ESP32 frequency conversion used a hardcoded lookup table instead of arithmetic
• ESP8266 flash mode was hardcoded as "dio" instead of reading from MCU config defaults
• ESP32-C2 had wrong default frequency (80m instead of 60m) because it was copy-pasted from ESP32

What to flag:

• Clock frequencies (e.g., 240000000, 80000000, 16000000) outside of const declarations
• Baud rates (e.g., 115200, 460800, 921600) not from board JSON upload.speed
• Flash frequencies/modes (e.g., "40m", "80m", "dio", "qio") not from config
• Board names or MCU names used for conditional logic instead of config-driven dispatch
• -D defines or compiler flags as string literals instead of from MCU config JSON
• Memory addresses, flash sizes, or pin numbers as magic numbers
• Frequency conversion tables (should be arithmetic or config-driven)
• Copy-pasted defaults from one platform applied to another

What is OK:

• Named const declarations with descriptive names
• Default values in Default impls or builder patterns
• Test fixtures and test data
• Log messages, error messages, format strings
• Standard protocol values (e.g., 0xFF for padding)
• Arithmetic on config-loaded values (e.g., hz / 1_000_000)

---

Check 2: Code Belongs in Core, Not Platform-Specific Crates

Rule: Logic that is not specific to a particular hardware platform should live in fbuild-core or shared modules in fbuild-build, not in individual platform directories.

Historical bugs this catches (commits fa142d7, 2813044, 14cc101):

• compile_with() was duplicated in every compiler — extracted to crate::compiler::compile_source()
• windows_temp_dir() was in a platform crate — moved to fbuild_core::response_file
• Build orchestration logic (discovery, compilation, linking, results) was duplicated 400+ lines per platform — extracted to pipeline.rs with BuildContext and run_sequential_build()
• Optimization flags were in common linker flags — moved to profile-specific configs
• Flag building helpers (build_c_flags, build_cpp_flags) were duplicated — centralized

What to flag:

• Utility functions in crates/fbuild-build/src/{platform}/ that have no platform-specific imports
• Data structures or parsing logic that could be reused across platforms
• Error types or trait definitions duplicated across platform modules
• Anything in a platform directory that doesn't reference platform-specific APIs, toolchain binaries, or MCU-specific config
• Helper functions for path manipulation, file discovery, or string processing in platform code
• Frequency conversion, flag merging, or profile selection logic duplicated across platforms

What is OK:

• Platform-specific implementations of shared traits (Compiler, Linker, BuildOrchestrator)
• Glue code wiring core logic to platform APIs
• Platform-specific error variants
• MCU config deserialization (each platform has its own JSON schema)

---

Check 3: Board JSON Quality

Rule: Board JSON files must be complete and consistent. Missing or incorrect fields cause silent build failures that are hard to diagnose.

Historical bugs this catches (commits 8a34526, c259f6f, 861c599, be3f6ed):

• ESP8266 board was missing critical defines array (ESP8266, LWIP, MMU flags) — builds compiled but linked wrong
• ESP32-C2/C61 needed separate MCU skeleton packages not handled by default — builds failed at link
• Teensy 31 was missing FPU flags (-mfloat-abi=hard, -mfpu=fpv4-sp-d16) — produced wrong code for Cortex-M4F
• ESP32-H2 needed f_image field because its f_flash value wasn't valid for esptool
• Toolchain sysroot includes were missing from orchestrators — headers not found

Required fields for ALL boards (check crates/fbuild-config/assets/boards/json/):

• id — must match filename (without .json)
• name — human-readable display name
• mcu — uppercase MCU name (e.g., "ESP32", "ATMEGA328P")
• platform — PlatformIO platform string (e.g., "espressif32", "atmelavr")
• frameworks — at least ["arduino"]
• fcpu — numeric Hz (e.g., 240000000), NOT a string
• ram and rom — numeric bytes
• build.core, build.mcu, build.f_cpu, build.variant
• build.extra_flags — must include -DARDUINO_<BOARD_DEFINE>
• upload.protocol and upload.speed

Additional required fields by platform:

• ESP32: build.f_flash, build.flash_mode, build.arduino.ldscript, build.arduino.partitions
• ESP32 with PSRAM: build.arduino.memory_type (e.g., "qio_opi")
• ESP32-H2: build.f_image when f_flash is not a valid esptool frequency
• Teensy ARM: FPU flags in MCU config if Cortex-M4F/M7
• AVR: build.arduino.ldscript or linker script path

f_cpu must end with "L" suffix in the build section (e.g., "240000000L")

What to flag:

• Missing required fields in new or modified board JSONs
• fcpu as string instead of number (or vice versa for build.f_cpu)
• id field not matching the filename
• Missing extra_flags -DARDUINO_ define
• ESP32 boards without f_flash/flash_mode
• Copy-pasted board JSON with wrong MCU, frequencies, or memory sizes from the source board

---

Check 4: MCU Config JSON Quality

Rule: Embedded MCU config JSONs (crates/fbuild-build/src/{platform}/configs/) drive the entire compilation. Errors here cause subtle build failures.

Historical bugs this catches (commits 7a8c7ba, 8a34526, 14cc101):

• ESP32-C2 default_flash_freq was "80m" (copy-pasted from ESP32) instead of "60m"
• ESP8266 MCU config was missing defines array entirely — compiler ran without critical defines
• Optimization flags were in compiler_flags.common instead of profiles.release — couldn't switch profiles

What to flag:

• Missing defines array in MCU config (especially for new platforms)
• Wrong default frequencies (copy-paste from another MCU config)
• Optimization flags (-Os, -O2, -flto) in compiler_flags.common instead of profiles
• Architecture-specific flags (e.g., -march=rv32imc) that don't match the MCU
• Missing esptool section for ESP32 variants
• Missing profiles.release and profiles.quick
• Linker flags missing -nostartfiles for bare-metal platforms

---

Check 5: Platform Orchestrator Completeness

Rule: When adding a new platform or modifying an orchestrator, ensure the full build pipeline is covered.

Historical bugs this catches (commits 391b857, 68a39f2, 10aa54f, be3f6ed):

• ESP8266 linker was missing generate_linker_scripts() — linker script template not preprocessed
• AVR/ESP32/Teensy orchestrators didn't handle projects with sketches at root (no src/ dir) — Arduino IDE projects failed
• Toolchain sysroot includes were missing from orchestrators — framework headers not found
• Local lib/ directory wasn't scanned for user libraries — libraries not compiled

What to flag in orchestrator changes:

• Missing toolchain.get_include_dirs() for sysroot includes
• No fallback from src/ to project root for source discovery
• Missing local lib/ directory scanning
• Linker scripts not generated/preprocessed before link step
• Missing MCU-specific skeleton package handling (ESP32-C2, C61 pattern)
• Missing compile_db.json generation
• No size reporting after build

---

Check 6: General Bug Scan

Rule: Look for common bugs and correctness issues in all changed code.

What to flag:

• unwrap() in non-test, non-CLI code (library crates should return Results)
• Silently swallowed errors (catch + continue pattern like compile_core() had)
• Off-by-one errors in loops or slicing
• Resource leaks (files, connections, locks not released)
• Race conditions or missing synchronization
• Logic errors (wrong operator, inverted condition, unreachable branches)
• Path handling that doesn't work on Windows (backslash vs forward slash)
• compile_source() or similar catching errors per-file and continuing — real errors surface later as misleading messages
• Response file handling that doesn't account for zccache wrapper
• Include path ordering issues (framework includes MUST come before sketch includes)

---

Output Format

For each issue found, report:

### [CHECK_NAME] file_path:line_number
**Severity**: high | medium | low
**Issue**: One-line description
**Suggestion**: How to fix it

If a check finds nothing, say so in one line. End with a summary:

## Summary
- Hardcoded values: N issues
- Core placement: N issues
- Board JSON quality: N issues
- MCU config quality: N issues
- Orchestrator completeness: N issues
- Bugs: N issues

If there are zero issues across all checks, just say "Code review passed - no issues found."