From ce8b92d8c02847ce3354c811c927e22f30a1fba6 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 25 Apr 2026 02:17:26 -0700 Subject: [PATCH] docs(library-selection): apply CodeRabbit follow-ups from PR #208 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five inline review findings from CodeRabbit on PR #208. All addressed. None changes runtime behaviour. ## Changes - `crates/fbuild-build/tests/teensylc_acceptance.rs` — drop substring fallback for `setup`/`loop` symbol probes. They're `extern "C"` from the sketch and exact `has_symbol(...)` is sufficient; the substring match was overly permissive (4-5-char tokens collide with mangled C++ names like `_ZN6Stream8setupXxx`). - `crates/CLAUDE.md` — add "Diagnostic subcommand exception" pattern to clarify that `clang-tidy` / `clang-query` / `iwyu` / `mcp` / `lnk` / `lib-select` intentionally bypass the daemon. Prevents future readers from concluding `lib-select` violates the "thin HTTP client" rule. - `docs/architecture/library-selection.md` — update top-of-doc status block and "Future work" list to reflect that PR #208 shipped Phase 6 acceptance gates and Phase 8.a `lib-select` CLI; only Phase 4 (zccache#130), Phase 7, and Phase 8.b cleanup remain. The "Tests" section now points at the actual `tests/*_acceptance.rs` files rather than describing them as future work. ## CodeRabbit comment 2 — false positive (no change) CodeRabbit flagged `use fbuild_packages::Framework;` in `crates/fbuild-cli/src/lib_select.rs:25` as unused. Verified: the `Framework` trait is required for method-call dispatch on the 12+ framework objects the file constructs (`get_libraries_dir()` is a trait method). Removing the import produces 12× E0599 errors. Kept. Refs: FastLED/fbuild#205, FastLED/fbuild#208 Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/CLAUDE.md | 2 ++ .../fbuild-build/tests/teensylc_acceptance.rs | 3 +-- docs/architecture/library-selection.md | 20 ++++++++++--------- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/CLAUDE.md b/crates/CLAUDE.md index 8fbfdae0..bace57fe 100644 --- a/crates/CLAUDE.md +++ b/crates/CLAUDE.md @@ -48,6 +48,8 @@ fbuild-test-support (test utilities) ────────────── **HTTP API boundary:** CLI sends JSON requests to daemon over HTTP. Build output streams via WebSocket. Serial monitor data streams via `/ws/serial-monitor`. All endpoints match the Python FastAPI daemon's contract. +**Diagnostic subcommand exception:** A small, growing set of `fbuild-cli` subcommands (`clang-tidy`, `clang-query`, `iwyu`, `mcp`, `lnk`, `lib-select`) run in-process and intentionally bypass the daemon. They are read-only diagnostics that don't need build orchestration, so a round-trip through the HTTP API would only add latency. The "thin HTTP client" rule still applies to every command that touches the build pipeline (`build`, `deploy`, `monitor`, `test-emu`, etc.). + **PyO3 consumer contract:** FastLED imports `SerialMonitor` as a Python context manager with `read_lines()`, `write()`, `write_json_rpc()`. The `fbuild-python` crate must preserve this API exactly. ## Current Status diff --git a/crates/fbuild-build/tests/teensylc_acceptance.rs b/crates/fbuild-build/tests/teensylc_acceptance.rs index 1b10f37d..3c5bd6d7 100644 --- a/crates/fbuild-build/tests/teensylc_acceptance.rs +++ b/crates/fbuild-build/tests/teensylc_acceptance.rs @@ -73,8 +73,7 @@ fn teensylc_blink_meets_205_acceptance_criteria() { } for required in ["setup", "loop"] { assert!( - probe.has_symbol(required).expect("symbol query") - || probe.has_symbol_containing(required).expect("symbol query"), + probe.has_symbol(required).expect("symbol query"), "A-11: required symbol '{required}' missing from ELF" ); } diff --git a/docs/architecture/library-selection.md b/docs/architecture/library-selection.md index a6a16464..aefa687e 100644 --- a/docs/architecture/library-selection.md +++ b/docs/architecture/library-selection.md @@ -1,8 +1,9 @@ # Library selection (LDF-style) > Status: foundation phases (0–3 + Phase 5 framework_libs delegation) landed -> in PR #207. Phase 4 (zccache memoization) tracked at zackees/zccache#130. -> Phase 6 acceptance gates and Phase 7 perf gates are follow-ups in #205. +> in PR #207. Phase 6 acceptance gates and Phase 8.a `lib-select` CLI landed +> in PR #208. Phase 4 (zccache memoization) tracked at zackees/zccache#130. +> Phase 7 perf gates and Phase 8.b cleanup remain follow-ups in `#205`. ## Why @@ -127,20 +128,21 @@ keys safe. - Resolver tests in `crates/fbuild-library-select/src/lib.rs` including the #204 regression guard, sort-stability, missing-include-dir tolerance, and same-basename-different-library disambiguation. -- Acceptance tests for AC#1 (teensyLC), AC#4 (stm32 SPI auto-discovery) - land in Phase 6 via `fbuild-test-support`'s `MiniFramework`, - `ElfProbe`, and `CompileDb` utilities. +- Acceptance gates for AC#1 (teensyLC) and AC#4 (stm32 SPI + auto-discovery) live in `crates/fbuild-build/tests/` + (`teensylc_acceptance.rs`, `stm32_acceptance.rs`). They are + `#[ignore]`'d by default and run by CI with `--ignored`. They use + `fbuild-test-support`'s `ElfProbe` and `CompileDb` utilities to probe + the produced firmware. ## Future work - **Phase 4** — zccache K/V memoization. Gated on zackees/zccache#130 shipping a versioned `KvStore` API and a 1.4.0 release; see `tasks/zccache-kv-design.md`. -- **Phase 6** — wire ELF + compile-DB probes through `fbuild-test-support` - into per-board acceptance tests, gating CI on AC#1..#4 from #205. - **Phase 7** — perf gates wired into `bench/fastled-examples`. -- **Phase 8** — `fbuild lib-select --explain` CLI subcommand and final - deletion of `framework_libs.rs` helpers. +- **Phase 8.b** — final deletion of any dead helpers in `framework_libs.rs` + once Phase 4 cache lands. ## References