// End-of-session code review checking for hardcoded values, core vs platform-specific placement, board JSON quality, and bugs in outstanding changes.
| 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 |
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.
# 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.
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):
defines array"dio" instead of reading from MCU config defaultsWhat to flag:
240000000, 80000000, 16000000) outside of const declarations115200, 460800, 921600) not from board JSON upload.speed"40m", "80m", "dio", "qio") not from config-D defines or compiler flags as string literals instead of from MCU config JSONWhat is OK:
const declarations with descriptive namesDefault impls or builder patterns0xFF for padding)hz / 1_000_000)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_filepipeline.rs with BuildContext and run_sequential_build()build_c_flags, build_cpp_flags) were duplicated — centralizedWhat to flag:
crates/fbuild-build/src/{platform}/ that have no platform-specific importsWhat is OK:
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):
defines array (ESP8266, LWIP, MMU flags) — builds compiled but linked wrong-mfloat-abi=hard, -mfpu=fpv4-sp-d16) — produced wrong code for Cortex-M4Ff_image field because its f_flash value wasn't valid for esptoolRequired 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.speedAdditional required fields by platform:
build.f_flash, build.flash_mode, build.arduino.ldscript, build.arduino.partitionsbuild.arduino.memory_type (e.g., "qio_opi")build.f_image when f_flash is not a valid esptool frequencybuild.arduino.ldscript or linker script pathf_cpu must end with "L" suffix in the build section (e.g., "240000000L")
What to flag:
fcpu as string instead of number (or vice versa for build.f_cpu)id field not matching the filenameextra_flags -DARDUINO_ definef_flash/flash_modeRule: 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):
default_flash_freq was "80m" (copy-pasted from ESP32) instead of "60m"defines array entirely — compiler ran without critical definescompiler_flags.common instead of profiles.release — couldn't switch profilesWhat to flag:
defines array in MCU config (especially for new platforms)-Os, -O2, -flto) in compiler_flags.common instead of profiles-march=rv32imc) that don't match the MCUesptool section for ESP32 variantsprofiles.release and profiles.quick-nostartfiles for bare-metal platformsRule: 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):
generate_linker_scripts() — linker script template not preprocessedsrc/ dir) — Arduino IDE projects failedlib/ directory wasn't scanned for user libraries — libraries not compiledWhat to flag in orchestrator changes:
toolchain.get_include_dirs() for sysroot includessrc/ to project root for source discoverylib/ directory scanningcompile_db.json generationRule: 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)compile_core() had)compile_source() or similar catching errors per-file and continuing — real errors surface later as misleading messagesFor 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."