Skip to content

docs(library-selection): apply CodeRabbit follow-ups from PR #208 #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zackees merged 1 commit into from
Apr 25, 2026
+14 −11
Merged

docs(library-selection): apply CodeRabbit follow-ups from PR #208 #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.).
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm whether mcp dispatches in-process or via HTTP to the daemon.
fd -t f 'mcp.rs' crates/fbuild-cli
echo "--- module header ---"
fd -t f 'mcp.rs' crates/fbuild-cli -x sed -n '1,40p' {}
echo "--- daemon/HTTP usage inside mcp module ---"
rg -nP -C2 '\b(DaemonClient|reqwest|http|base_url|/api/|post|get_json)\b' crates/fbuild-cli/src/mcp.rs crates/fbuild-cli/src/mcp 2>/dev/null
echo "--- dispatch from main/cli ---"
rg -nP -C3 '\bmcp\b' crates/fbuild-cli/src/main.rs crates/fbuild-cli/src/lib.rs 2>/dev/null

Repository: FastLED/fbuild

Length of output: 3844

Remove mcp from the diagnostic-bypass exception list in CLAUDE.md line 51.

The mcp subcommand acts as a thin HTTP client to the daemon, not an in-process diagnostic that bypasses it. Its own module documentation states it "Translates MCP tool/resource/prompt calls into HTTP requests to the running daemon," and the implementation uses DaemonClient throughout to communicate with the daemon. Including it in the exception contradicts both the documented behavior and the "thin HTTP client" rule being reaffirmed. The diagnostic-bypass list should contain only: clang-tidy, clang-query, iwyu, lnk, lib-select.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/CLAUDE.md` at line 51, Update the diagnostic-bypass exception list in
CLAUDE.md by removing the `mcp` entry so the list only contains `clang-tidy`,
`clang-query`, `iwyu`, `lnk`, `lib-select`; `mcp` should not be listed because
it is a thin HTTP client that uses `DaemonClient` rather than an in-process
diagnostic. Locate the sentence containing "clang-tidy, clang-query, iwyu, mcp,
lnk, lib-select" and edit it to drop `mcp`, preserving punctuation and wording
consistency with the surrounding paragraph.


**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
Expand Down
3 changes: 1 addition & 2 deletions crates/fbuild-build/tests/teensylc_acceptance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
}
Expand Down
20 changes: 11 additions & 9 deletions docs/architecture/library-selection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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

Expand Down
Loading