Repository navigation
Conversation
CMake on Windows writes compile_commands.json commands for CreateProcess; shlex.split ate their backslashes, so every real compiler probe and check_build's body check failed there.
…posix build_doc's tests call main() in-process: a #!/bin/sh stub on PATH cannot run on Windows, so the real sphinx-build ran instead.
The CLI tests answer git and gh in-process: #!/bin/sh stubs on PATH cannot run on Windows, so the real git ran there.
No fcntl there: the started record is locked with msvcrt on a byte past its content, and its temporary name goes only after the close. Receipt keys and messages are posix, so a receipt pins the firmware a run stages; rsync gets local paths relative to the checkout, since it reads a drive letter as a remote host.
….exe, release Windows locks
…nt the split dialects
…ep POSIX stdout as given
…dio only on Windows
… a process on POSIX
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 26.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 188 functions across 27 files. (1 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: enabling the agent skill scripts to run on Windows. Description check The description directly explains the Windows compatibility failures, implemented fixes, and test results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 26.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 188 functions across 27 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
✨ Simplify code
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
 [Medium impact] The PR appears safe to merge; no new actionable issues were found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 25
.claude/skills/hil-contract/SKILL.md: 1
113-113: LGTM!.claude/skills/hil-contract/scripts/hil_remote.py: 2
626-647: Windows lock acquisition is correct, and the retry loop is bounded only byblock.The code saves and restores the file position and retries only on
EACCESandEDEADLOCK. The blocking loop has no timeout. This matchesflock(LOCK_EX)semantics forpublish_locked, so no change is needed.
601-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Write the started record and the done record as UTF-8.
Other reads and writes in this file now use
encoding='utf-8'.write_jsonandpublish_lockedstill callwrite_textwith the locale encoding. The data is ASCII-only today, becausejson.dumpsescapes non-ASCII characters by default. The change is therefore not urgent. Set the encoding explicitly so the files stay consistent with the new UTF-8 reads..claude/test/test_hil_remote.py: 1
30-31: LGTM!Also applies to: 124-126, 526-606
test/hil/test/test_hil_remote.py: 1
119-155: LGTM!Also applies to: 373-374, 398-398
tools/family_json.py: 3
99-137: Windowssplit_commandmatches the Microsoft C runtime rules.I traced the backslash and quote handling and the
""rule inside quotes. I did not find a defect. The tests cover trailing backslashes, embedded quotes and quoted paths with spaces.
343-369: Windows lock handling looks correct.The loop retries
LK_NBLCKonly onEACCESandEDEADLOCK, and re-raises other errors. The unlock runs infinally. Seeking to byte 0 before each lock and unlock keeps the locked range the same.
72-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Chain the
Failureto the originalValueError.Ruff B904 flags that
raise Failure(...)inside theexcept ValueErrorclause hides the cause. Usefrom e. The same applies to the otherraisestatements inexceptclauses.Proposed fix
- raise Failure(f'{path.relative_to(ROOT).as_posix()} is not valid JSON ({e}); fix it by hand') + raise Failure(f'{path.relative_to(ROOT).as_posix()} is not valid JSON ({e}); fix it by hand') from eSource: Linters/SAST tools
.claude/skills/build/scripts/coverage_audit.py: 2
233-234: The LTOnmname is wrong when the compiler has a.exesuffix with a non-gccname.The code appends
-nmafter stripping.exe. This is correct forarm-none-eabi-gcc.exe. Forclang.exeorcc.exe, no matching-nmtool exists. In that caserunraisesOSErrorandis_livereturnsTrue. This is the safe fallback, so no change is needed.
51-64: Cross-drive handling is correct.
relrecords the path inother_driveand returnsNone. This matches the POSIX "outside the checkout" semantics.cmd_graphclears the set per board and reports it.test/hil/test/test_family_json.py: 2
119-150: LGTM!
153-185: LGTM!.claude/test/test_coverage_audit.py: 1
117-127: Therunclosure binds the loop variablecalls.Ruff B023 flags this. The closure runs inside the same iteration, so the behavior is correct. No change is required. Pass
callsas a default argument only if you want to silence the warning..claude/skills/build-doc/scripts/gen_doc.py: 1
24-24: LGTM!Also applies to: 111-111, 128-130, 135-135
.claude/skills/build/scripts/check_build.py: 1
435-435: LGTM!Also applies to: 662-662, 746-746
.claude/skills/make-release/scripts/release.py: 1
62-62: LGTM!Also applies to: 72-72, 78-78, 83-83, 86-86, 91-91, 124-124, 146-148, 153-153
.claude/skills/update-sponsor/update_sponsor.py: 1
65-65: LGTM!Also applies to: 191-203, 411-413
.claude/skills/usbtest/scripts/kernel_src.py: 1
167-168: LGTM!Also applies to: 191-191
tools/build_utils.py: 1
613-614: LGTM!tools/get_deps.py: 1
422-422: LGTM!.claude/test/test_build_doc.py: 1
5-102: LGTM!.claude/test/test_gen_doc.py: 1
42-43: LGTM!Also applies to: 54-54, 73-87, 106-107, 121-122
.claude/test/test_release.py: 1
93-104: LGTM!Also applies to: 119-161, 180-208
.claude/test/test_update_sponsor.py: 1
1-62: LGTM!.claude/test/test_usbtest_kernel_src.py: 1
180-183: LGTM!Also applies to: 201-207, 209-215
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
edc5aa24-a431-4d86-93db-df7d01bea0be
📒 Files selected for processing: 27
.claude/skills/build-doc/scripts/gen_doc.py.claude/skills/build/scripts/check_build.py.claude/skills/build/scripts/coverage_audit.py.claude/skills/hil-contract/SKILL.md.claude/skills/hil-contract/scripts/hil_remote.py.claude/skills/make-release/scripts/release.py.claude/skills/update-sponsor/update_sponsor.py.claude/skills/usb-kernel-recover/scripts/usb_recover.sh.claude/skills/usbtest/scripts/kernel_src.py.claude/skills/usbtest/scripts/run_case.py.claude/skills/usbtest/scripts/wedge_drill.py.claude/test/test_build_doc.py.claude/test/test_coverage_audit.py.claude/test/test_gen_doc.py.claude/test/test_hil_remote.py.claude/test/test_release.py.claude/test/test_update_sponsor.py.claude/test/test_usb_recover.py.claude/test/test_usbtest_host.py.claude/test/test_usbtest_kernel_src.py.claude/test/test_usbtest_run_case.py.claude/test/test_usbtest_wedge_drill.pytest/hil/test/test_family_json.pytest/hil/test/test_hil_remote.pytools/build_utils.pytools/family_json.pytools/get_deps.py
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Hardware-in-the-loop (HIL) Test Reporthfp-iar✅ 25 passed · ❌ 4 failed · ⚪ 0 skipped · blank not run
hfp.json✅ 16 passed · ❌ 13 failed · ⚪ 0 skipped · blank not run
tinyusb-esp.json✅ 36 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 398 passed · ❌ 28 failed · ⚪ 19 skipped · blank not run
|
Code sizeBaseline: 687ab47 (exact) https://github.com/hathach/tinyusb/actions/runs/39144371176
1090 builds on 33 boards compared, 0 changed no changes |
|
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
Reviewed the whole change at e6af283: 40 new finding(s) (40 open); open thread claims: 3 refuted.
CI: red, all 4 failing jobs rig-side: the same HIL cells fail on master (runs 39133069767, 39110007058, 39017404166), and the one ESP32-S3 dd stall did not recur.
| Board | HIL | Regression |
|---|---|---|
| nrf52840dk | pass | none |
.claude/skills/build/scripts/coverage_audit.py:213: low: run() still decodes ninja -t deps/targets/query and nm output with the locale code page (text=True), as does open(script, errors='replace') at line 126.
check_build.pymoved the same tool output toencoding='utf-8', errors='replace'in this diff.- Impact: on Windows a non-ASCII path in ninja's output decodes differently from the UTF-8
compile_commands.jsonit is matched against.
.claude/skills/usbtest/scripts/run_case.py:11: nit: The new non-Linux refusal (exit 2) is missing from the documented list of exit-2 refusals.
run_case.py:11: the docstring, also shown by--help, omits it.wedge_drill.pydefers to that list, and usbtestSKILL.mdline 43 does not mention it.- Impact: a caller cannot learn from the contract that a non-Linux host is refused.
.claude/test/test_hil_remote.py:392: low: The sidecar UTF-8 test cannot detect a missing encoding='utf-8'.
test_hil_remote.py:526:json.dumpsdefaults toensure_ascii=True, so the check mark is written as the ASCII escape✔and any codec decodes it. (Reported as line 392; the code is at 526-527.)- Impact: it would not catch a regression in
sidecar_matches(hil_remote.py:231). - Fix:
json.dumps(..., ensure_ascii=False).
.claude/test/test_update_sponsor.py:649: low: Same gap as the test_release stub: it returns str whatever the call's text or encoding, so dropping encoding= from gh() or codespell_collisions() would not fail here.
- In the real script
gh()would then return bytes to itsstrcallers, and the codespell parse would raise. - On a UTF-8 locale a switch to
text=Trueis hidden too; the "whatever the locale" check only bites on Windows.
.claude/test/test_usb_recover.py:728: low: The skip check excludes only the System32 WSL launcher, not the Store alias.
test_usb_recover.py(NotLinux.test_every_verb_is_refused_before_it_runs; reported as line 728): a Store-installed WSL registersbash.exein%LOCALAPPDATA%\Microsoft\WindowsApps.- Impact: if that comes first on PATH, the test runs WSL bash with a Windows path and fails (exit 127, or
unameis Linux) instead of skipping. Windows only.
.claude/test/test_usbtest_kernel_src.py:812: low: test_utf8_source_prints_as_utf8_whatever_the_locale depends on the caller's locale on POSIX.
test_usbtest_kernel_src.py:201(reported as 812):kernel_src.py:167-168reconfigures stdout only onnt, andrun_scriptpasses the wholeos.environto the child.- Impact:
PYTHONIOENCODING=latin-1orasciimakes it fail. - Fix: pin
PYTHONIOENCODING=utf-8in this test on POSIX, or skip it there.
…e_sponsor and tools/ edits, document the off-Linux refusal
…en Windows refuses the hidden unlink
…amily_json, make the Windows tests bite on POSIX
…ame board files posix
|
Review-body findings, outside the diff's lines:
|
|
# Conflicts: # .claude/skills/usb-kernel-recover/scripts/usb_recover.sh # .claude/test/test_usb_recover.py
This branch has not been deployed
2 participants
On Windows the agent skill scripts failed or gave wrong answers:
check_build.pyreported every driver "unverified: preprocessing failed":tools/family_json.pysplit CMake'sWindows-quoted compile commands POSIX-style, so the compiler path lost its backslashes. The same split broke the
real-compiler probe, so
family_json fixcould not add or refresh rows.split_commandnow follows the Windows Cruntime's rules there; POSIX keeps
shlex.split.coverage_audit.pydropped every source: native separators never matched ninja's/, andrelpathacross drivesraised.
hil_remote.pycrashed on import (fcntl) and refused every receipt (backslash keys); rsync is now resolved asrsync.exeonPATH, and every refusal comes before the lease.gen_doc.pycrashed on sixfamily.cfiles (GBK decode);update_sponsor.py's codespell guard was silently off(the temp file could not be reopened);
release.pyandkernel_src.pyread and printed text in the localeencoding.
run_case.py,wedge_drill.pyandusb_recover.shcrashed or misreported off Linux; they now refuse throughtheir normal contract (the JSON verdict line, exit 2).
Visible on POSIX, on purpose:
check_builddecodes compiler output witherrors='replace'(a crash became areplacement character); a missing rsync is refused before the lease instead of after the remote wipe; the usbtest
scripts and
usb_recover.shrefuse on macOS too.Tests on Windows:
.claude/test227 run, OK, 48 skipped (151 run, 12 failures, 5 errors before);test/hil/test/test_family_json.py26 OK;test/hil/test/test_hil_remote.py37 OK; each under CPython 3.12 with andwithout
-X utf8and MSYS2's python3.check_build.py --scope src/portable/wch/dcd_ch32_usbfs.c: pass, nothinguncovered, 6 boards. pre-commit over the changed files: codespell and claude-scripts pass; hil-test,
change-impact-test, drivers-coverage and membrowse-targets fail on Windows with the same tests at master. Not run on
Linux; no HIL.