| 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
git diff HEAD
git log --oneline -10
git diff HEAD~5..HEAD
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 namemcu — 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 stringram and rom — numeric bytesbuild.core, build.mcu, build.f_cpu, build.variantbuild.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."
