Uh oh!
There was an error while loading. Please reload this page.
test(ide): add headless clangd --check parity harness (#1076) - #1205
Merged
Conversation
Ignored integration test proving the include/define/target correctness the IDE depends on: generates tests/platform/uno's compile DB in-process (compiledb_only path, real translate_for_clang + swap_ino_entries_for_raw pipeline), asserts the raw-.ino entry replaced the generated .ino.cpp entry with -x c++ -include <prelude> and the prelude exists, then runs `clangd --check=<uno.ino> --compile-commands-dir=<project>` and fails on any error-severity diagnostic. Skips cleanly with guidance when clangd is not on PATH (fbuild deliberately does not distribute clangd; Zed manages its own). Runbook added to docs/DEVELOPMENT.md. Part of #1076 (acceptance harness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an ignored Rust acceptance test that generates an AVR IDE compile database, validates raw ChangesClangd parity validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-build/tests/clangd_check_parity.rs`:
- Around line 213-227: Add an assertion in the raw-.ino argument checks near the
existing `-include` assertion to require the `-x`, `c++` argument pair,
including validation that `-x` has a following value. Keep the existing prelude
lookup and assertions unchanged.
- Around line 241-251: Update the clangd process setup in the parity test to
include the required allow-direct-spawn marker and configure the tokio Command
with kill_on_drop(true) before invoking output inside under_timeout, preserving
the existing arguments and stdio configuration.
🪄 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 Plus
Run ID: e6a089ef-1c37-4e9e-a6b1-c33c02812dae
📒 Files selected for processing (2)
crates/fbuild-build/tests/clangd_check_parity.rsdocs/DEVELOPMENT.md
Comment on lines
+213
to
+227
| let arguments = ino_entry["arguments"] | ||
| .as_array() | ||
| .expect("entry.arguments must be an array"); | ||
| let arg_strs: Vec<&str> = arguments.iter().filter_map(|a| a.as_str()).collect(); | ||
| assert!( | ||
| arg_strs.contains(&"-include"), | ||
| "raw-.ino entry must carry -include <prelude> per the #1197 prelude design: {:?}", | ||
| arg_strs | ||
| ); | ||
| let prelude_index = arg_strs | ||
| .iter() | ||
| .position(|a| *a == "-include") | ||
| .expect("checked above") | ||
| + 1; | ||
| let prelude_path = arg_strs[prelude_index]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the required -x c++ argument pair.
The test only verifies -include; a regression dropping language forcing would pass this structural check despite the stated raw-.ino contract.
Proposed assertion
let arg_strs: Vec<&str> = arguments.iter().filter_map(|a| a.as_str()).collect();
+ assert!(+ arg_strs+ .windows(2)+ .any(|pair| pair[0] == "-x" && pair[1] == "c++"),+ "raw-.ino entry must carry -x c++: {:?}",+ arg_strs+ );
assert!(
arg_strs.contains(&"-include"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let arguments = ino_entry["arguments"] | |
| .as_array() | |
| .expect("entry.arguments must be an array"); | |
| let arg_strs:Vec<&str> = arguments.iter().filter_map(|a| a.as_str()).collect(); | |
| assert!( | |
| arg_strs.contains(&"-include"), | |
| "raw-.ino entry must carry -include <prelude> per the #1197 prelude design: {:?}", | |
| arg_strs | |
| ); | |
| let prelude_index = arg_strs | |
| .iter() | |
| .position(|a| *a == "-include") | |
| .expect("checked above") | |
| + 1; | |
| let prelude_path = arg_strs[prelude_index]; | |
| let arguments = ino_entry["arguments"] | |
| .as_array() | |
| .expect("entry.arguments must be an array"); | |
| let arg_strs:Vec<&str> = arguments.iter().filter_map(|a| a.as_str()).collect(); | |
| assert!( | |
| arg_strs | |
| .windows(2) | |
| .any(|pair| pair[0] == "-x" && pair[1] == "c++"), | |
| "raw-.ino entry must carry -x c++: {:?}", | |
| arg_strs | |
| ); | |
| assert!( | |
| arg_strs.contains(&"-include"), | |
| "raw-.ino entry must carry -include <prelude> per the `#1197` prelude design: {:?}", | |
| arg_strs | |
| ); | |
| let prelude_index = arg_strs | |
| .iter() | |
| .position(|a| *a == "-include") | |
| .expect("checked above") | |
| 1; | |
| let prelude_path = arg_strs[prelude_index]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-build/tests/clangd_check_parity.rs` around lines 213 - 227, Add
an assertion in the raw-.ino argument checks near the existing `-include`
assertion to require the `-x`, `c++` argument pair, including validation that
`-x` has a following value. Keep the existing prelude lookup and assertions
unchanged.
Comment on lines
+241
to
+251
| let mut cmd = tokio::process::Command::new(&clangd_path); | ||
| cmd.arg(format!("--check={sketch_path}")) | ||
| .arg(format!("--compile-commands-dir={}", project_dir.display())) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()); | ||
| let output = under_timeout(async { | ||
| cmd.output() | ||
| .await | ||
| .expect("failed to spawn clangd — found on PATH but could not execute") | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Confirm the declared Tokio dependency and inspect existing approved spawn patterns.
fd '^Cargo\.toml$' crates -x sh -c 'echo "--- $1"; rg -n -C2 "tokio" "$1"' _ {}
rg -n -C2 'allow-direct-spawn:|run_command(?:_passthrough)?' cratesRepository: FastLED/fbuild
Length of output: 6441
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"## repository files matching clangd / direct spawn / run_command"
rg -n -C2 'allow-direct-spawn:|tokio::process::Command|direct_spawn|run_command|under_timeout|kill_on_drop|process::Command'.||trueechoecho"## target file outline"
ast-grep outline crates/fbuild-build/tests/clangd_check_parity.rs --view expanded 2>/dev/null ||trueechoecho"## target file relevant sections"
wc -l crates/fbuild-build/tests/clangd_check_parity.rs
sed -n '1,320p' crates/fbuild-build/tests/clangd_check_parity.rs
echoecho"## Cargo workspace dependency features"
rg -n -C3 '^\[workspace\.dependencies\]|tokio|process' Cargo.toml crates -g 'Cargo.toml'Repository: FastLED/fbuild
Length of output: 50371
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"## ci/find_direct_subprocess.py direct-spawn logic"
sed -n '1,170p' ci/find_direct_subprocess.py
echoecho"## target clangd code"
sed -n '220,270p' crates/fbuild-build/tests/clangd_check_parity.rs
echoecho"## fbuild-core subprocess/contained spawn kill-on-drop behavior"
rg -n -C3 'kill_on_drop|spawn_contained|Command::new|tokio::process::Command' crates/fbuild-core crates/fbuild-daemon/src/handlers/operations/build.rs crates/fbuild-daemon/src/handlers/emulator/shared.rs crates/fbuild-daemon/src/handlers/emulator/runners.rsRepository: FastLED/fbuild
Length of output: 26726
Make the clangd spawn allowlisted and cancellation-safe.
Add the required allow-direct-spawn: marker on this spawn, and set kill_on_drop(true) so dropping/timing out the awaited cmd.output() future doesn’t leave clangd --check running.
Proposed fix
- let mut cmd = tokio::process::Command::new(&clangd_path);+ // allow-direct-spawn: capture clangd diagnostics for the clangd-check-parity acceptance harness.+ let mut cmd = tokio_process::Command::new(&clangd_path);
cmd.arg(format!("--check={sketch_path}"))
.arg(format!("--compile-commands-dir={}", project_dir.display()))🧰 Tools
🪛 GitHub Actions: Lint subprocess spawns / 0_Lint subprocess spawns.txt
[error] 241-241: Direct spawn detected without an allow-direct-spawn: <reason> marker. New direct subprocess call at line 241 (tokio::process::Command::new(&clangd_path)); annotate with a one-line reason or route via fbuild_core::subprocess::{run_command,run_command_passthrough}.
🪛 GitHub Actions: Lint subprocess spawns / Lint subprocess spawns
[error] 241-241: New direct spawn without an allow-direct-spawn: <reason> marker. Suggested route: fbuild_core::subprocess::{run_command,run_command_passthrough} or add a one-line allow reason (#141). Offending code: let mut cmd = tokio::process::Command::new(&clangd_path);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-build/tests/clangd_check_parity.rs` around lines 241 - 251,
Update the clangd process setup in the parity test to include the required
allow-direct-spawn marker and configure the tokio Command with
kill_on_drop(true) before invoking output inside under_timeout, preserving the
existing arguments and stdio configuration.
Source: Pipeline failures
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 freeto 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.
The corrected #1076 plan's acceptance harness: CI can't launch Zed, so IDE correctness is proven headlessly with
clangd --check.#[ignore = "requires installed toolchains and clangd on PATH (#1076 parity harness)"]integration test incrates/fbuild-build/tests/clangd_check_parity.rs: generatestests/platform/uno's compile DB in-process (compiledb_only, exercising the realtranslate_for_clang→swap_ino_entries_for_rawpipeline from feat(ide): clangd/compile-DB foundations for fbuild ide (#1076 Phase 0) #1197), asserts the raw-.inoentry replaced the generated.ino.cppentry (core regression check) with-x c++ -include <prelude>and that the prelude exists, then runsclangd --check=<uno.ino>and fails on anyerror:-severity diagnostic (documented empty allowlist).docs/DEVELOPMENT.md.Verification honesty
This machine has no clangd, so the skip path is the live result. The harness's assertions were proven load-bearing with two temporary fake-clangd stubs (never committed): a
0 errorsstub → full harness passes end-to-end (DB generated from the cached AVR toolchain, entry/prelude assertions exercised, correct clangd args); anerror:-emitting stub → test fails with the offending diagnostic. Normal suite unaffected (test reportsignored).Part of #1076.
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation