docs(library-selection): apply CodeRabbit follow-ups from PR #208#209
Merged
docs(library-selection): apply CodeRabbit follow-ups from PR #208#209
Conversation
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: #205, #208 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes consist of three documentation and test refinements: architectural documentation clarifying that fbuild-cli diagnostic subcommands execute in-process, a tightened symbol-match assertion in the Teensy acceptance test, and updated library-selection roadmap documentation with detailed test descriptions and phase status updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/CLAUDE.md`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a83c84b-9a87-44f8-b70e-a3087bca9ed1
📒 Files selected for processing (3)
crates/CLAUDE.mdcrates/fbuild-build/tests/teensylc_acceptance.rsdocs/architecture/library-selection.md
|
|
||
| **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.). |
There was a problem hiding this comment.
🧩 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/nullRepository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the five inline CodeRabbit findings on PR #208 (now merged). No runtime changes.
Changes
CR-2 — false positive
CodeRabbit flagged `use fbuild_packages::Framework;` in `crates/fbuild-cli/src/lib_select.rs` as unused. Verified locally: the `Framework` trait is needed for `get_libraries_dir()` method-call dispatch on the 12 framework objects the file constructs. Removing it produces `E0599` errors. Kept.
Verification
Refs: #205, #208
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests