fix: doc-reference lint, review follow-ups, preflight portability, and homebrew release trigger - #1857
Merged
Merged
Conversation
Three checks in lint_docs.py fail on master. Two single-backtick spans contain a single quote, which Doxygen treats as ending the span, so they need double backticks. The testing.md cross-reference to the new pitfalls section needs a matching {#selection-and-execution-pitfalls} anchor on that header.
Written with assistance from Claude Code.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes doc-reference lint failures introduced in #1855 by adjusting inline-code delimiters that contain single quotes and by adding a missing documentation anchor for an intra-doc link.
Changes:
- Add a
{#...}anchor to the “Selection and Execution Pitfalls” header so#selection-and-execution-pitfallsresolves. - Switch two inline-code spans from single backticks to double backticks to avoid Doxygen’s single-quote parsing issue.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/documentation/testing.md | Adds the missing selection-and-execution-pitfalls anchor to satisfy cross-reference lint. |
| docs/documentation/gpuParallelization.md | Uses double-backtick inline code for a macro call containing single quotes to satisfy lint. |
| docs/documentation/contributing.md | Uses double-backtick inline code for a directive containing single quotes to satisfy lint. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Point the device-routine lint at documentation that exists. The error message now names the Silent-Failure Traps section of gpuParallelization.md, which is repo-local and survives changes to the external reproducer repo, and that section gains the {#silent-failure-traps} anchor Doxygen needs. The docstring keeps the full context: the docs section, MFC issue 1815, and a full URL to the compiler-bugs reproducer, whose path was verified to exist.
Also fix trailing whitespace and three grammar slips in .claude/CLAUDE.md.
Written with assistance from Claude Code.
Two portability bugs made preflight.sh silently skip the node probe outside Linux, which is the failure mode the script exists to prevent. find -printf is GNU-only, so on a BSD find the discovery pipeline failed into 2>/dev/null, returned nothing, and the script reported 'no syscheck binary' and exited healthy; it now uses -exec ls -t {} + instead. Under set -u, bash 3.2 treats the empty array expansion "${launcher[@]}" as an unbound variable, so the probe aborted before running; it now uses ${launcher[@]+"${launcher[@]}"}.
The toolchain lint reported 13 failures across the five preflight test files on macOS, and two further tests passed only because a skipped probe returns the same healthy status the assertions expected. All 29 now pass, and ./mfc.sh precheck is green end to end. Verified against both GNU findutils 4.11.0 with bash 5.3.15 and the system BSD find with bash 3.2.57.
Written with assistance from Claude Code.
GitHub skips push-triggered workflows when the head commit message carries [skip ci], and MFC tags land on the coverage-map commit, which does. v5.6.0 and v5.7.0 therefore published without a formula update: v5.6.0 was rescued by a manual workflow_dispatch three minutes later, and v5.7.0 has had none. v5.6.1 fired normally because its tag pointed at an ordinary commit. docker.yml was unaffected throughout because a release event is not a push event, so this moves homebrew-release.yml to the same trigger. The version now comes from github.event.release.tag_name rather than GITHUB_REF, and prereleases are skipped rather than failing the X.Y.Z check. Written with assistance from Claude Code.
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
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.
Summary
Fixes the
Lint Toolchain / Precheckfailure that #1855 introduced on master (run 34670170525). Three errors, all fromlint_docs.py:Doxygen treats a single quote inside a single-backtick span as ending the span, so both spans become double-backticked. The
testing.mdcross-reference gets its matching{#selection-and-execution-pitfalls}anchor on the target header.Three lines changed, no prose edits.
Verification
python3 toolchain/mfc/lint_docs.pyexits 0, and./mfc.sh precheckreports "Doc references are valid" along with formatting, spelling, source lint, parameter docs, and example validation.Precheck's toolchain-lint step fails locally with 13
test_preflight*failures, but those reproduce identically on unmodifiedupstream/master(git stashand re-run), are macOS-specific probing tests aroundsrun/mpirun, and passed in the same CI run that caught the doc errors. They are unrelated to this change.Review follow-ups from #1855
Copilot's review of #1855 raised four points; two are taken here.
Lint pointers now name documentation that exists. The error message in
check_device_routine_element_argssaidsee sbryngelson/compiler-bugs cce/acc-routine-element-by-reference, which is not actionable from a CI log. It now names the Silent-Failure Traps section ofdocs/documentation/gpuParallelization.md— repo-local, so it survives any change to the external repo's layout. That section gains the{#silent-failure-traps}anchor Doxygen requires, so the reference is real rather than nominal. The docstring keeps the full trail: the docs section, #1815, and a full URL to the reproducer, whose path was checked to exist rather than assumed..claude/CLAUDE.md: trailing whitespace removed from two lines, and three grammar slips fixed ("CFD result need verification PR is correct", "follow existing the source's", "while being correctness").Not taken: the review also asked to preserve a "previously-local breadcrumb" in the docstring. That breadcrumb was
.claude/rules/common-pitfalls.md, the file #1855 removed, so there was nothing left to point at — which is what prompted the repoint above.Verification
python3 toolchain/mfc/lint_docs.pyexits 0.check_device_routine_element_argsstill runs against the tree and reports 0 findings, unchanged../mfc.sh lintreports the same 13test_preflight*failures as unmodifiedupstream/master, so this change adds none.Preflight portability
Unrelated to the docs above, but folded in here rather than split into its own PR.
./mfc.sh precheckcould not pass on macOS: its toolchain-lint step reported 13 failures across the five*preflight*test files. Both causes are real bugs in.github/scripts/preflight.sh, not test-environment noise.find -printfis GNU-only.newest_syscheckranfind ... -printf '%T@ %p\n' 2>/dev/null. A BSD find rejects-printf, the error is swallowed by the redirect, discovery returns nothing, and the script prints "no syscheck binary under build/install; skipping node probe" and exits healthy. It now uses-exec ls -t {} +, which also removes thesort/cutpipeline.set -uplus an empty array under bash 3.2.run_probe "${launcher[@]}"aborts withlauncher[@]: unbound variableon the bash macOS ships; bash 4.4 changed this, so Linux never sees it. Now${launcher[@]+"${launcher[@]}"}.The test fixture gains
lsin its hermetic PATH, since the script now calls it.Worth noting for review: two tests were passing vacuously before.
test_does_not_report_a_node_fault_merely_because_pmix_printed_a_warningandtest_a_missing_launcher_is_not_blamed_on_the_nodeassert a healthy exit, and a skipped probe also exits healthy. They began failing once discovery worked and passed again after the bash fix, so the prior state was 13 red plus at least 2 false greens.Verified on both toolchains rather than on the BSD side only:
-printf-exec ls -t {} +/usr/bin/findThe five preflight files give 29 passed against system BSD find with bash 3.2.57, and
test_preflight.pygives 12 passed with the fixture temporarily pointed at GNU findutils 4.11.0 with bash 5.3.15, as a stand-in for the CI stack. That fixture change was reverted../mfc.sh precheckis now green end to end, and this commit passed the pre-commit hook without--no-verify.Homebrew release trigger
v5.7.0 published without a formula update.
homebrew-release.ymltriggered onpush: tags: v*, and GitHub skips push-triggered workflows when the head commit message carries[skip ci]— which MFC's tags land on, because they point at the coverage-map commit:test: refresh coverage map [skip ci]workflow_dispatch3 min after publishfix(container): disable -Mfprelaxed…test: refresh coverage map [skip ci]docker.ymlwas unaffected across all three because it triggers onrelease: [published], and a release event is not a push event — its v5.7.0 run exists and is only waiting on a runner. This moveshomebrew-release.ymlto that same trigger, so the formula update no longer depends on what the tagged commit message happens to say.Two follow-on changes: the version now comes from
github.event.release.tag_name(with anyvstripped) rather thanGITHUB_REF, and prereleases are skipped via a job-levelifrather than failing theX.Y.Zregex with a red job.actionlintreports the same 21 findings before and after this edit — all pre-existing shellcheckSC2086/SC2129notes on the pre-existing$GITHUB_STEP_SUMMARYsteps — so the change introduces none.Note this does not retroactively publish v5.7.0's formula; that still needs a one-off
gh workflow run homebrew-release.yml -f version=5.7.0.Note on tooling
Written with assistance from Claude Code. The original error was mine: the checks were verified by calling three functions out of
lint_docs.pyby name rather than running the script, so the backtick and anchor checks never executed.