Add CI lint gate and local precheck command#1122
Add CI lint gate and local precheck command#1122sbryngelson merged 16 commits intoMFlowCode:masterfrom
Conversation
- Add lint-gate job to test.yml that runs fast checks (formatting, spelling, linting) before expensive test matrix and HPC jobs start - Add concurrency groups to test.yml, coverage.yml, cleanliness.yml, and bench.yml to cancel superseded runs on new pushes - Add ./mfc.sh precheck command for local CI validation before pushing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughAdds top-level concurrency to multiple GitHub Actions workflows; introduces a CI lint-gate job and a new bench Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant BenchWF as Bench Workflow
end
rect rgba(200,255,200,0.5)
participant Wait as wait-for-tests job
end
rect rgba(255,200,200,0.5)
participant CI as GitHub Test Suite
end
rect rgba(255,255,200,0.5)
participant Lint as lint-gate job
end
BenchWF->>Wait: start wait-for-tests
Wait->>CI: poll test-suite job statuses
CI-->>Wait: return statuses (passed/failed/pending)
Wait->>Lint: query lint-gate result
Lint-->>Wait: return lint outcome
alt all required checks passed
Wait-->>BenchWF: allow `self` job to proceed
else failures or timeout
Wait-->>BenchWF: fail or block downstream
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
codeant-ai
bot
added
the
size:L
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
High-level Suggestion
The linting logic is duplicated in both the test.yml CI workflow and the new precheck.sh script. To improve maintainability and prevent divergence, the CI workflow should call ./mfc.sh precheck directly, making the script the single source of truth. [High-level, importance: 8]
Solution Walkthrough:
Before:
# In .github/workflows/test.yml
jobs:
lint-gate:
steps:
- name: Check Formatting
run: |
./mfc.sh format ...
git diff --exit-code ...
- name: Spell Check
run: ./mfc.sh spelling
- name: Lint Toolchain
run: ./mfc.sh lint
- name: Lint Source - No Raw Directives
run: ! grep ...
# ... and other source lint steps
# In toolchain/bootstrap/precheck.sh
# ... similar checks for formatting, spelling, toolchain, and source linting are defined again.
After:
# In .github/workflows/test.yml
jobs:
lint-gate:
steps:
- name: Clone
...
- name: Initialize MFC
run: ./mfc.sh init
- name: Run Pre-push Checks
run: ./mfc.sh precheck
# In toolchain/bootstrap/precheck.sh
# This script becomes the single source of truth for all checks.
# ... checks for formatting, spelling, linting, and source code ...
| #!/bin/bash | ||
| set -e | ||
| set -o pipefail |
There was a problem hiding this comment.
Suggestion: Source the logging.sh script at the beginning of precheck.sh. This is necessary to define the log, ok, and error helper functions used throughout the script. [possible issue, importance: 9]
| #!/bin/bash | |
| set -e | |
| set -o pipefail | |
| #!/bin/bash | |
| set -e | |
| set -o pipefail | |
| . "$(pwd)/toolchain/bootstrap/logging.sh" |
| # 1. Check formatting | ||
| log "[$CYAN 1/4$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." | ||
| # Capture state before formatting | ||
| BEFORE_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) | ||
| if ! ./mfc.sh format -j "$JOBS" > /dev/null 2>&1; then | ||
| error "Formatting check failed to run." | ||
| FAILED=1 | ||
| else | ||
| # Check if formatting changed any Fortran/Python files | ||
| AFTER_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) | ||
| if [ "$BEFORE_HASH" != "$AFTER_HASH" ]; then | ||
| error "Code is not formatted. Run$MAGENTA ./mfc.sh format$COLOR_RESET to fix." | ||
| echo "" | ||
| git diff --stat -- '*.f90' '*.fpp' '*.py' 2>/dev/null || true | ||
| echo "" | ||
| FAILED=1 | ||
| else | ||
| ok "Formatting check passed." | ||
| fi | ||
| fi |
There was a problem hiding this comment.
✅ Suggestion: Simplify the formatting check by replacing the md5sum hash comparison with a git diff --exit-code check. This removes a dependency and makes the script more portable and robust. [general, importance: 7]
| # 1. Check formatting | |
| log "[$CYAN 1/4$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." | |
| # Capture state before formatting | |
| BEFORE_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) | |
| if ! ./mfc.sh format -j "$JOBS" > /dev/null 2>&1; then | |
| error "Formatting check failed to run." | |
| FAILED=1 | |
| else | |
| # Check if formatting changed any Fortran/Python files | |
| AFTER_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) | |
| if [ "$BEFORE_HASH" != "$AFTER_HASH" ]; then | |
| error "Code is not formatted. Run$MAGENTA ./mfc.sh format$COLOR_RESET to fix." | |
| echo "" | |
| git diff --stat -- '*.f90' '*.fpp' '*.py' 2>/dev/null || true | |
| echo "" | |
| FAILED=1 | |
| else | |
| ok "Formatting check passed." | |
| fi | |
| fi | |
| # 1. Check formatting | |
| log "[$CYAN 1/4$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." | |
| if ! ./mfc.sh format -j "$JOBS" > /dev/null 2>&1; then | |
| error "Formatting check failed to run." | |
| FAILED=1 | |
| elif ! git diff -- '*.f90' '*.fpp' '*.py' > /dev/null; then | |
| error "Code is not formatted. Run$MAGENTA ./mfc.sh format$COLOR_RESET to fix." | |
| echo "" | |
| git diff --stat -- '*.f90' '*.fpp' '*.py' 2>/dev/null || true | |
| echo "" | |
| FAILED=1 | |
| else | |
| ok "Formatting check passed." | |
| fi |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| elif [ "$1" '==' "precheck" ]; then | ||
| . "$(pwd)/toolchain/bootstrap/python.sh" | ||
|
|
||
| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 |
There was a problem hiding this comment.
Suggestion: The precheck command always exits with status 0, ignoring any non-zero status from the precheck script or Python bootstrap, so failures in the underlying checks will not cause this wrapper to fail, breaking its use as a gating/pre-push check. [logic error]
Severity Level: Critical 🚨
- ❌ ./mfc.sh precheck reports success despite failures.
- ⚠️ Local pre-push validation may be misleading.
- ⚠️ Developers may push changes with lint errors.| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 | |
| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit $? |
Steps of Reproduction ✅
1. From the repository root, run the wrapper command: `./mfc.sh precheck`. This enters the
precheck branch in `mfc.sh` (see `mfc.sh` hunk lines 43-46: the `elif [ "$1" '=='
"precheck" ]` block).
2. `mfc.sh` sources the Python bootstrap at `mfc.sh` line 44 (`.
"$(pwd)/toolchain/bootstrap/python.sh"`), then executes the precheck script by sourcing
`toolchain/bootstrap/precheck.sh` at `mfc.sh` line 46 (`shift; .
"$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0`).
3. If `toolchain/bootstrap/precheck.sh` detects failures and returns/non-zero (for example
a linter or formatting check exits 1), that non-zero status is lost because `mfc.sh`
immediately executes `exit 0` on line 46.
4. Verify by running a deliberately-failing precheck (or a small wrapper
`toolchain/bootstrap/precheck.sh` that does `exit 1`): after `./mfc.sh precheck` observe
the shell exit code is 0 (`echo $?`), proving the wrapper masks failures and reports
success.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mfc.sh
**Line:** 46:46
**Comment:**
*Logic Error: The precheck command always exits with status 0, ignoring any non-zero status from the precheck script or Python bootstrap, so failures in the underlying checks will not cause this wrapper to fail, breaking its use as a gating/pre-push check.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| set -e | ||
| set -o pipefail | ||
|
|
||
| # Function to display help message |
There was a problem hiding this comment.
Suggestion: The script relies on helper functions (log, ok, error) and color variables (MAGENTA, CYAN, COLOR_RESET) that are not defined anywhere in this file, so if precheck.sh is executed directly in a clean shell (rather than being sourced from a wrapper that defines them), the first call to log will result in "command not found" and the script will exit due to set -e. Adding safe fallback definitions for these helpers ensures the script runs correctly both standalone and when invoked via a parent script that may already define them. [possible bug]
Severity Level: Major ⚠️
- ❌ Local `./mfc.sh precheck` may abort with "command not found".
- ⚠️ Developer convenience: standalone usage inconsistent.| # Function to display help message | |
| # Provide fallbacks for logging functions and color variables when not pre-defined | |
| : "${MAGENTA:=}" | |
| : "${CYAN:=}" | |
| : "${COLOR_RESET:=}" | |
| if ! declare -f log >/dev/null 2>&1; then | |
| log() { printf '%s\n' "$*"; } | |
| fi | |
| if ! declare -f ok >/dev/null 2>&1; then | |
| ok() { printf '%s\n' "$*"; } | |
| fi | |
| if ! declare -f error >/dev/null 2>&1; then | |
| error() { printf '%s\n' "$*" >&2; } | |
| fi | |
Steps of Reproduction ✅
1. Invoke the script directly in a clean shell: `./toolchain/bootstrap/precheck.sh` (file
`toolchain/bootstrap/precheck.sh`, execution starts at line 1; `set -e` and `set -o
pipefail` are on lines 2-3).
2. Execution reaches the first logging call at line 40: `log "Running$MAGENTA
precheck$COLOR_RESET (same checks as CI lint-gate)..."` (see final file lines around 40).
The script expects `log`, `ok`, `error`, and color variables to exist.
3. If those functions/variables are not defined by the higher-level wrapper (e.g.
`./mfc.sh`) and the script is executed standalone, the shell reports `./precheck.sh: line
40: log: command not found` and, because `set -e` is enabled (line 2), the script exits
immediately.
4. Observable outcome: precheck aborts with "command not found" instead of running checks.
The suggested fallback definitions ensure the script prints sensible messages when run
standalone.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/bootstrap/precheck.sh
**Line:** 5:5
**Comment:**
*Possible Bug: The script relies on helper functions (`log`, `ok`, `error`) and color variables (`MAGENTA`, `CYAN`, `COLOR_RESET`) that are not defined anywhere in this file, so if `precheck.sh` is executed directly in a clean shell (rather than being sourced from a wrapper that defines them), the first call to `log` will result in "command not found" and the script will exit due to `set -e`. Adding safe fallback definitions for these helpers ensures the script runs correctly both standalone and when invoked via a parent script that may already define them.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| JOBS="$2" | ||
| shift | ||
| ;; | ||
| -h | --help) |
There was a problem hiding this comment.
✅ Suggestion: The -j/--jobs argument handling assumes a value is always present; running the script with "-j" (without a value) will cause an extra shift on an empty argument list, triggering "shift: shift count out of range" and immediate exit due to set -e, instead of a clear error about the missing jobs value. The fix is to validate that a value exists for -j/--jobs and to shift exactly two arguments (option and value) when it is provided. [logic error]
Severity Level: Major ⚠️
- ❌ Local precheck crashes on `./mfc.sh precheck -j` misuse.
- ⚠️ Developer pre-push validation interrupted, confusing error messages.| JOBS="$2" | |
| shift | |
| ;; | |
| -h | --help) | |
| if [[ $# -lt 2 ]] || [[ "$2" == -* ]]; then | |
| echo "Precheck: -j|--jobs requires a positive integer argument." | |
| exit 1 | |
| fi | |
| JOBS="$2" | |
| shift 2 | |
| continue | |
| ;; | |
| -h|--help) |
Steps of Reproduction ✅
1. From the repository root, run the precheck script directly:
`./toolchain/bootstrap/precheck.sh -j` (this file is `toolchain/bootstrap/precheck.sh`,
see lines 1-4 for `set -e` and the argument loop at lines 20-36).
2. Execution enters the while-case at line 20; the `-j|--jobs)` branch executes lines
22-25 which assign `JOBS="$2"` and perform a single `shift` (line 24), expecting a value
in `$2`.
3. Because no value was provided, `$2` is empty; the script continues and reaches the
unconditional `shift` at line 35. Bash then raises `shift: shift count out of range`
(shift with no positional parameters), and because `set -e` is active (line 2), the script
exits immediately with an error.
4. Observable outcome: abrupt exit with shell `shift` error message instead of a clear,
actionable error about the missing `-j` argument. The fix in the suggestion prevents the
out-of-range shift by validating `$2` and shifting by 2 (lines 22-25 area).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/bootstrap/precheck.sh
**Line:** 23:26
**Comment:**
*Logic Error: The -j/--jobs argument handling assumes a value is always present; running the script with "-j" (without a value) will cause an extra shift on an empty argument list, triggering "shift: shift count out of range" and immediate exit due to `set -e`, instead of a clear error about the missing jobs value. The fix is to validate that a value exists for -j/--jobs and to shift exactly two arguments (option and value) when it is provided.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | ||
| error "Found double precision intrinsics. Use generic intrinsics." | ||
| SOURCE_FAILED=1 |
There was a problem hiding this comment.
Suggestion: The double-precision intrinsic check uses grep -qiR ... ./src/* 2>/dev/null inside an if without inspecting grep's exit status, so if grep fails for reasons like a missing ./src directory or read errors (exit status >1), the script silently treats it as "no matches" and later reports "Source lint passed," giving a false sense of success. The fix is to distinguish between "no matches" (exit status 1) and real grep errors (>1), and to mark the source lint as failed in the latter case instead of ignoring them. [logic error]
Severity Level: Major ⚠️
- ❌ Source lint can falsely report success when scanning fails.
- ⚠️ Local precheck trustworthiness reduced for wrong directory.| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/*; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| else | |
| grep_status=$? | |
| if [ "$grep_status" -gt 1 ]; then | |
| error "Source lint failed: could not scan files for double precision intrinsics." | |
| SOURCE_FAILED=1 | |
| FAILED=1 | |
| fi |
Steps of Reproduction ✅
1. From repository root, rename or remove the `src` directory: `mv src src.bak` (this
causes `./src/*` glob to fail). The double-precision check is at
`toolchain/bootstrap/precheck.sh` lines 92-96 (`# Check for double precision intrinsics`
through `fi`).
2. Run `./toolchain/bootstrap/precheck.sh`. The script executes the `grep -qiR ... ./src/*
2>/dev/null` command at line 93; because `./src/*` doesn't expand to files, `grep` exits
with status 2 (error). stderr was redirected to /dev/null, so no error message is shown.
3. The `if` condition tests only for a successful match (exit status 0). On grep exit
status 2, the `if` body is skipped and `SOURCE_FAILED` remains unchanged. Later (lines
104-108) the script reports "Source lint passed." despite the scan failing.
4. Observable outcome: false-positive "Source lint passed" when files were not scanned.
The suggested change inspects grep's exit status and treats scanning errors as failures.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/bootstrap/precheck.sh
**Line:** 93:95
**Comment:**
*Logic Error: The double-precision intrinsic check uses `grep -qiR ... ./src/* 2>/dev/null` inside an `if` without inspecting grep's exit status, so if grep fails for reasons like a missing `./src` directory or read errors (exit status >1), the script silently treats it as "no matches" and later reports "Source lint passed," giving a false sense of success. The fix is to distinguish between "no matches" (exit status 1) and real grep errors (>1), and to mark the source lint as failed in the latter case instead of ignoring them.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![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
🤖 Fix all issues with AI agents
In `@toolchain/bootstrap/precheck.sh`:
- Around line 46-52: BEFORE_HASH and AFTER_HASH use the non-portable md5sum;
make the script detect available checksum tool and use it portably: add a
detection block that sets a MD5_CMD variable to "md5sum" if present, otherwise
to "md5 -r" (or fail with a clear error), then replace uses of "md5sum | cut -d'
' -f1" with a pipeline using $MD5_CMD that yields only the hash (accounting for
md5's output format) so the BEFORE_HASH and AFTER_HASH assignments continue to
produce the same single-hash string on Linux and macOS.
🧹 Nitpick comments (3)
mfc.sh (1)
43-46: Quote$@to prevent word splitting.The static analysis tool correctly flags that
$@should be double-quoted to prevent re-splitting of arguments containing spaces. While this follows the existing pattern in the file (lines 26, 30, 34, 36, 42), it's worth fixing for robustness.Proposed fix
elif [ "$1" '==' "precheck" ]; then . "$(pwd)/toolchain/bootstrap/python.sh" - shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 + shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" "$@"; exit 0 fi.github/workflows/bench.yml (1)
29-32: Consider adding lint-gate dependency for consistency.Unlike the test.yml workflow, the benchmark job doesn't depend on lint-gate. Since benchmarks are expensive HPC jobs, you may want to gate them behind lint checks as well to avoid wasting resources on code that will fail basic formatting/linting.
This could be intentional given the different trigger conditions (approved reviews or specific users), so flagging for consideration only.
Optional: Add lint-gate dependency
self: name: "${{ matrix.name }} (${{ matrix.device }}${{ matrix.interface != 'none' && format('-{0}', matrix.interface) || '' }})" if: ${{ github.repository=='MFlowCode/MFC' && needs.file-changes.outputs.checkall=='true' && ((github.event_name=='pull_request_review' && github.event.review.state=='approved') || (github.event_name=='pull_request' && (github.event.pull_request.user.login=='sbryngelson' || github.event.pull_request.user.login=='wilfonba'))) }} - needs: file-changes + needs: [lint-gate, file-changes]This would also require adding the lint-gate job definition to bench.yml.
toolchain/bootstrap/precheck.sh (1)
20-36: Edge case:-jwithout a value.If
-jis provided as the last argument without a value,$2will be empty, andJOBSwill be set to an empty string, which could cause issues downstream.Proposed fix with validation
while [[ $# -gt 0 ]]; do case "$1" in -j|--jobs) + if [[ -z "$2" || "$2" == -* ]]; then + echo "Error: -j/--jobs requires a numeric argument." + exit 1 + fi JOBS="$2" shift ;;
There was a problem hiding this comment.
Pull request overview
This PR adds a fast-failing lint gate to the CI pipeline and provides a local precheck command for developers to validate their code before pushing.
Changes:
- Adds a
lint-gatejob to test.yml that runs formatting, spelling, and linting checks before expensive test matrix and HPC jobs start, providing faster feedback (1-2 minutes) for common issues - Adds concurrency groups to test.yml, coverage.yml, cleanliness.yml, and bench.yml workflows to automatically cancel superseded runs when new commits are pushed to the same branch
- Adds a new
./mfc.sh precheckcommand that mirrors the CI lint-gate checks for local validation before pushing
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/bootstrap/precheck.sh | New script implementing local precheck functionality with formatting, spelling, and source lint checks |
| mfc.sh | Integrates the new precheck command into the MFC toolchain |
| .github/workflows/test.yml | Adds lint-gate job that must pass before test matrix runs, plus concurrency cancellation |
| .github/workflows/coverage.yml | Adds concurrency group to cancel superseded runs |
| .github/workflows/cleanliness.yml | Adds concurrency group to cancel superseded runs |
| .github/workflows/bench.yml | Adds concurrency group to cancel superseded runs |
|
|
||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| -j|--jobs) |
There was a problem hiding this comment.
Missing validation for the --jobs argument value. If the user runs './mfc.sh precheck -j' without providing a number, JOBS will be set to an empty string, potentially causing issues when passed to './mfc.sh format'. Add a check to ensure $2 is provided and is a valid number before using it.
| -j|--jobs) | |
| -j|--jobs) | |
| if [[ -z "$2" || "$2" == -* ]]; then | |
| echo "Precheck: option '$1' requires a positive integer argument." | |
| exit 1 | |
| fi | |
| if ! [[ "$2" =~ ^[1-9][0-9]*$ ]]; then | |
| echo "Precheck: jobs value '$2' is not a valid positive integer." | |
| exit 1 | |
| fi |
| if grep -qiR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/* 2>/dev/null; then | ||
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | ||
| SOURCE_FAILED=1 | ||
| fi | ||
|
|
||
| # Check for double precision intrinsics | ||
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | ||
| error "Found double precision intrinsics. Use generic intrinsics." | ||
| SOURCE_FAILED=1 | ||
| fi | ||
|
|
||
| # Check for junk code patterns | ||
| if grep -qiR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/* 2>/dev/null; then |
There was a problem hiding this comment.
The stderr redirection '2>/dev/null' on grep commands differs from the CI implementation in test.yml (lines 38, 42, 46), which doesn't suppress stderr. This could cause the local precheck to silently hide errors that would be visible in CI, creating inconsistent behavior. Remove the '2>/dev/null' redirection to match CI behavior.
| if grep -qiR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/* 2>/dev/null; then | |
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for double precision intrinsics | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for junk code patterns | |
| if grep -qiR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/* 2>/dev/null; then | |
| if grep -qiR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/*; then | |
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for double precision intrinsics | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/*; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for junk code patterns | |
| if grep -qiR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/*; then |
toolchain/bootstrap/precheck.sh
Outdated
| if ./mfc.sh spelling > /dev/null 2>&1; then | ||
| ok "Spell check passed." | ||
| else | ||
| error "Spell check failed." |
There was a problem hiding this comment.
The spell check error message on line 69 doesn't provide guidance on how to see details or fix the issue. The lint error message on line 78 includes helpful guidance. Update the spell check error message to match this pattern, suggesting users run './mfc.sh spelling' for details.
| error "Spell check failed." | |
| error "Spell check failed. Run$MAGENTA ./mfc.sh spelling$COLOR_RESET for details." |
| if grep -qiR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/* 2>/dev/null; then | ||
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | ||
| SOURCE_FAILED=1 | ||
| fi | ||
|
|
||
| # Check for double precision intrinsics | ||
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | ||
| error "Found double precision intrinsics. Use generic intrinsics." | ||
| SOURCE_FAILED=1 | ||
| fi | ||
|
|
||
| # Check for junk code patterns | ||
| if grep -qiR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/* 2>/dev/null; then |
There was a problem hiding this comment.
The error messages for source lint failures don't provide enough actionable information. Users are told that raw directives or forbidden patterns were found, but not shown where they are or how to find them. Consider either showing the grep output or providing guidance on how to locate the offending code.
| if grep -qiR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/* 2>/dev/null; then | |
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for double precision intrinsics | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for junk code patterns | |
| if grep -qiR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/* 2>/dev/null; then | |
| if grep -niR '!\$acc\|!\$omp' --exclude="parallel_macros.fpp" --exclude="acc_macros.fpp" --exclude="omp_macros.fpp" --exclude="shared_parallel_macros.fpp" --exclude="syscheck.fpp" ./src/* 2>/dev/null; then | |
| error "Found raw OpenACC/OpenMP directives. Use macros instead." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for double precision intrinsics | |
| if grep -niR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Check for junk code patterns | |
| if grep -niR -e '\.\.\.' -e '\-\-\-' -e '===' ./src/* 2>/dev/null; then |
|
|
||
| - name: Lint Source - No Double Precision Intrinsics | ||
| run: | | ||
| ! grep -iR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* |
There was a problem hiding this comment.
The pattern 'double\ precision' (with backslash before space) will match a literal backslash followed by a space, not just a space. In grep basic regex without -E flag, the backslash before a space has no special meaning. The pattern should be 'double precision' (without backslash) to match "double precision" with a space. This affects both the CI workflow and local precheck script.
| ! grep -iR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* | |
| ! grep -iR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* |
| fi | ||
|
|
||
| # Check for double precision intrinsics | ||
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then |
There was a problem hiding this comment.
The pattern 'double\ precision' (with backslash before space) will match a literal backslash followed by a space, not just a space. In grep basic regex, the backslash before a space has no special meaning. The pattern should be 'double precision' (without backslash) to match "double precision" with a space. This matches the same issue in the CI workflow.
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then |
toolchain/bootstrap/precheck.sh
Outdated
| # Check if formatting changed any Fortran/Python files | ||
| AFTER_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) | ||
| if [ "$BEFORE_HASH" != "$AFTER_HASH" ]; then | ||
| error "Code is not formatted. Run$MAGENTA ./mfc.sh format$COLOR_RESET to fix." |
There was a problem hiding this comment.
The error message says "Run ./mfc.sh format to fix" but the formatting has already been applied by the precheck script itself (line 47). The files are already formatted, so the user just needs to review and stage the changes. Consider updating the message to something like "Code has been auto-formatted. Review and commit the changes." to better reflect what happened.
| error "Code is not formatted. Run$MAGENTA ./mfc.sh format$COLOR_RESET to fix." | |
| error "Code has been auto-formatted. Review and commit the changes." |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="toolchain/bootstrap/precheck.sh">
<violation number="1" location="toolchain/bootstrap/precheck.sh:22">
P3: Validate that --jobs has a value before shifting so missing arguments produce a clear error instead of exiting with a shift error.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 8
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
toolchain/bootstrap/precheck.sh
Outdated
| # 1. Check formatting | ||
| log "[$CYAN 1/4$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." | ||
| # Capture state before formatting | ||
| BEFORE_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | md5sum | cut -d' ' -f1) |
There was a problem hiding this comment.
md5sum unavailable on macOS breaks local precheck
Medium Severity
The precheck.sh script uses md5sum to hash git diff output, but md5sum is a GNU coreutils command not available on macOS by default. macOS provides md5 instead, or requires installing coreutils via Homebrew. Since precheck is designed for local development use and the script has set -e enabled, macOS developers will see a "command not found" error when running ./mfc.sh precheck. A cross-platform alternative like shasum -a 256 or a conditional using md5 -q for macOS would resolve this.
Additional Locations (1)
- Add cross-platform hash function (macOS uses md5, Linux uses md5sum) - Validate -j/--jobs argument (require value, must be numeric) - Improve error messages with actionable guidance - Clarify that formatting has been auto-applied when check fails Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/bootstrap/precheck.sh`:
- Around line 113-116: The existing grep pattern uses the token "d0" which is
too narrow; update the check in the if-condition (the grep command that
currently searches for 'double_precision\|... \|d0') to use an extended regex
that matches Fortran double-precision literals with a 'd' exponent (e.g. 1d-2,
5.0d-11) and switch to extended regex mode (grep -E or --extended-regexp).
Replace the 'd0' fragment with a pattern such as
'([0-9]+(\.[0-9]*)?|\.[0-9]+)d[+-]?[0-9]+' so hardcoded double literals like
1d-2 and 5.0d-11 are detected while avoiding accidental matches to non-literals.
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | ||
| error "Found double precision intrinsics. Use generic intrinsics." | ||
| SOURCE_FAILED=1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the precheck.sh file
find . -name "precheck.sh" -type fRepository: MFlowCode/MFC
Length of output: 90
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '113,116p' ./toolchain/bootstrap/precheck.shRepository: MFlowCode/MFC
Length of output: 433
🏁 Script executed:
# Check context around those lines
sed -n '100,130p' ./toolchain/bootstrap/precheck.shRepository: MFlowCode/MFC
Length of output: 1229
🏁 Script executed:
# Check for potential false positives from 'd0' pattern in fortran source files
echo "=== Potential false positives (d0 in variable names or non-literal contexts) ==="
grep -riH 'd0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src --include="*.f90" --include="*.fpp" 2>/dev/null | grep -v '\.[0-9]*[dD]' | head -30 || echo "No false positive candidates found"Repository: MFlowCode/MFC
Length of output: 197
🏁 Script executed:
# Check for actual double precision literals (what the pattern should catch)
echo "=== Actual double precision literals (intended matches) ==="
grep -rEH '[0-9]+\.[0-9]*[dD][+-]?[0-9]+|[0-9]+[dD][+-]?[0-9]+' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src --include="*.f90" --include="*.fpp" 2>/dev/null | head -20 || echo "No double precision literals found"Repository: MFlowCode/MFC
Length of output: 701
🏁 Script executed:
# Count total d0 matches to understand the scope
echo "=== Total 'd0' matches in source ==="
grep -ric 'd0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src --include="*.f90" --include="*.fpp" 2>/dev/null | awk -F: '{sum+=$2} END {print "Total d0 matches:", sum}'Repository: MFlowCode/MFC
Length of output: 113
The d0 pattern is too broad and fails to catch most double precision literals.
The pattern d0 is intended to catch hardcoded double precision literals like 1.0d0, but it fails to match actual double precision literals in the codebase such as 1d-2 and 5.0d-11 (which contain d- not d0). Additionally, the pattern would match variable names like reserved0 if they weren't already excluded.
The codebase contains double precision literals that should be flagged:
src/pre_process/m_start_up.fpp:1d-2src/simulation/m_bubbles_EL.fpp:5.0d-11
Use a more specific regex to match floating-point literals with d exponent notation and ensure all violations are caught:
Proposed fix
-if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then
+if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then
error "Found double precision intrinsics. Use generic intrinsics."
SOURCE_FAILED=1
fi
+# Separate check for double precision literals (e.g., 1.0d0, 1d-2, 5.0d-11)
+if grep -qER '[0-9]+\.?[0-9]*[dD][+-]?[0-9]+' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then
+ error "Found double precision literals (e.g., 1.0d0). Use wp kind parameter."
+ SOURCE_FAILED=1
+fi📝 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.
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh\|d0' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| if grep -qiR 'double_precision\|dsqrt\|dexp\|dlog\|dble\|dabs\|double\ precision\|real(8)\|real(4)\|dprod\|dmin\|dmax\|dfloat\|dreal\|dcos\|dsin\|dtan\|dsign\|dtanh\|dsinh\|dcosh' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision intrinsics. Use generic intrinsics." | |
| SOURCE_FAILED=1 | |
| fi | |
| # Separate check for double precision literals (e.g., 1.0d0, 1d-2, 5.0d-11) | |
| if grep -qER '[0-9]+\.?[0-9]*[dD][+-]?[0-9]+' --exclude-dir=syscheck --exclude="*nvtx*" --exclude="*precision_select*" ./src/* 2>/dev/null; then | |
| error "Found double precision literals (e.g., 1.0d0). Use wp kind parameter." | |
| SOURCE_FAILED=1 | |
| fi |
🤖 Prompt for AI Agents
In `@toolchain/bootstrap/precheck.sh` around lines 113 - 116, The existing grep
pattern uses the token "d0" which is too narrow; update the check in the
if-condition (the grep command that currently searches for
'double_precision\|... \|d0') to use an extended regex that matches Fortran
double-precision literals with a 'd' exponent (e.g. 1d-2, 5.0d-11) and switch to
extended regex mode (grep -E or --extended-regexp). Replace the 'd0' fragment
with a pattern such as '([0-9]+(\.[0-9]*)?|\.[0-9]+)d[+-]?[0-9]+' so hardcoded
double literals like 1d-2 and 5.0d-11 are detected while avoiding accidental
matches to non-literals.
Add wait-for-tests job that polls GitHub API to ensure: - Lint Gate passes first (fast fail) - All Github test jobs complete successfully - Only then do benchmark jobs start This prevents wasting HPC resources on benchmarking code that fails tests, while preserving the existing maintainer approval gate. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/bench.yml:
- Around line 37-79: Replace the current check-runs polling with logic that
scopes to the latest Test Suite workflow run and inspects that run's jobs: use
the Actions workflow-runs endpoint to find the most recent run for the Test
Suite (scoped to the PR/head SHA stored in SHA), then call the workflow run jobs
endpoint to get job-level conclusions for that run; look up the "Lint Gate" job
conclusion and all jobs whose name starts with "Github" from that run (not
aggregated check-runs), and immediately fail if "Lint Gate" is any terminal
non-success (failure, cancelled, timed_out, skipped, neutral, etc.), or if any
"Github"-prefixed job has a terminal non-success conclusion; only wait/poll
while the latest run has non-terminal in-progress/null conclusions, and loop by
re-querying the latest workflow run id each iteration so reruns don’t get
blocked by earlier runs.
- Around line 29-36: Update the workflow job "wait-for-tests" to remove the
unnecessary permissions entry granting actions: read and instead grant only
checks: read so the "Wait for Test Suite to Pass" step can call the Checks API;
locate the permissions block for the wait-for-tests job (associated with the job
name wait-for-tests and the step named "Wait for Test Suite to Pass") and
replace actions: read with checks: read.
| wait-for-tests: | ||
| name: Wait for Test Suite | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Wait for Test Suite to Pass | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/bench.ymlRepository: MFlowCode/MFC
Length of output: 8041
🏁 Script executed:
# Check for any actions API calls in the workflow
rg "actions/runs|actions/workflows" .github/workflows/bench.yml
# Also verify the check-runs API endpoint requirements
echo "=== Checking all gh api calls in wait-for-tests job ==="
sed -n '36,79p' .github/workflows/bench.yml | grep -E "gh api|--jq"Repository: MFlowCode/MFC
Length of output: 704
🏁 Script executed:
# Check if there are any other workflow files that might show permission patterns
find .github/workflows -name "*.yml" -type f | head -10 | xargs grep -l "permissions:" 2>/dev/null
# Also check if this is a public repo to understand default token permissions
git remote -v | head -5Repository: MFlowCode/MFC
Length of output: 345
🏁 Script executed:
# Check what permissions are used in those files
echo "=== deploy-tap.yml permissions ==="
grep -A 5 "permissions:" .github/workflows/deploy-tap.yml | head -15
echo -e "\n=== line-count.yml permissions ==="
grep -A 5 "permissions:" .github/workflows/line-count.yml | head -15
# Also check if the repo has a default permissions setting at workflow level
grep -n "permissions:" .github/workflows/bench.ymlRepository: MFlowCode/MFC
Length of output: 511
🌐 Web query:
GitHub Actions default GITHUB_TOKEN permissions check-runs API public repository
💡 Result:
- Default
GITHUB_TOKENpermissions (public repo): By default (for newly created repos in a personal account),GITHUB_TOKENis restricted to read access forcontentsandpackagesonly; org repos inherit the org’s setting. You can switch the repo/org default to a more permissive “read/write for all scopes” in Settings → Actions → General → Workflow permissions. [1] - Check Runs API needs
checks: write: Creating/managing check runs via the REST API requires a token with Checks (write) permission, and GitHub notes that write access to Checks via the REST API is only available to GitHub Apps (not OAuth apps/authenticated users). [2] - So, why it fails by default: With the restricted default,
GITHUB_TOKENwon’t havechecks: write, so calls like Create a check run will be denied unless you grant that permission. [1][2] - Fix in workflow: explicitly grant it:
(Any permission you don’t list is set to
permissions: checks: write
none.) [3] - Fork PR caveat (public repos): workflows triggered from pull requests from public forks get a read-only token regardless of these settings, so creating/updating check runs may still be blocked in that scenario. [3]
Sources: [1] GitHub Docs – repository Actions settings / default GITHUB_TOKEN permissions; [2] GitHub Docs – REST API “check runs” (Create a check run) permission notes; [3] GitHub Changelog – permissions: syntax + public-fork PR read-only behavior.
Remove unnecessary actions: read permission; add only checks: read.
The job makes read-only calls to the Checks API endpoint (repos/{owner}/{repo}/commits/{ref}/check-runs), not the Actions API. With restricted default GITHUB_TOKEN permissions, the job will fail without explicit read access to checks. Add checks: read only:
Corrected permissions block
wait-for-tests:
name: Wait for Test Suite
runs-on: ubuntu-latest
+ permissions:
+ checks: read📝 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.
| wait-for-tests: | |
| name: Wait for Test Suite | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Wait for Test Suite to Pass | |
| env: | |
| GH_TOKEN: ${{ github.token }} | |
| run: | | |
| wait-for-tests: | |
| name: Wait for Test Suite | |
| runs-on: ubuntu-latest | |
| permissions: | |
| checks: read | |
| steps: | |
| - name: Wait for Test Suite to Pass | |
| env: | |
| GH_TOKEN: ${{ github.token }} | |
| run: | |
🤖 Prompt for AI Agents
In @.github/workflows/bench.yml around lines 29 - 36, Update the workflow job
"wait-for-tests" to remove the unnecessary permissions entry granting actions:
read and instead grant only checks: read so the "Wait for Test Suite to Pass"
step can call the Checks API; locate the permissions block for the
wait-for-tests job (associated with the job name wait-for-tests and the step
named "Wait for Test Suite to Pass") and replace actions: read with checks:
read.
| echo "Waiting for Test Suite workflow to complete..." | ||
| SHA="${{ github.event.pull_request.head.sha || github.sha }}" | ||
|
|
||
| # Poll every 60 seconds for up to 3 hours | ||
| for i in $(seq 1 180); do | ||
| # Get the Test Suite workflow runs for this commit | ||
| STATUS=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \ | ||
| --jq '.check_runs[] | select(.name == "Lint Gate") | .conclusion' | head -1) | ||
|
|
||
| if [ "$STATUS" = "success" ]; then | ||
| echo "Lint Gate passed. Checking test jobs..." | ||
|
|
||
| # Check if any Github test jobs failed | ||
| FAILED=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \ | ||
| --jq '[.check_runs[] | select(.name | startswith("Github")) | select(.conclusion == "failure")] | length') | ||
|
|
||
| if [ "$FAILED" != "0" ]; then | ||
| echo "::error::Test Suite has failing jobs. Benchmarks will not run." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check if Github tests are still running | ||
| PENDING=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \ | ||
| --jq '[.check_runs[] | select(.name | startswith("Github")) | select(.conclusion == null)] | length') | ||
|
|
||
| if [ "$PENDING" = "0" ]; then | ||
| echo "All Test Suite jobs completed successfully!" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Tests still running ($PENDING pending)..." | ||
| elif [ "$STATUS" = "failure" ]; then | ||
| echo "::error::Lint Gate failed. Benchmarks will not run." | ||
| exit 1 | ||
| else | ||
| echo "Lint Gate status: ${STATUS:-pending}..." | ||
| fi | ||
|
|
||
| sleep 60 | ||
| done | ||
|
|
||
| echo "::error::Timeout waiting for Test Suite to complete." | ||
| exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/test.yml | head -100Repository: MFlowCode/MFC
Length of output: 3596
🏁 Script executed:
rg -n "name:" .github/workflows/test.yml | grep -E "(Lint|Github|GitHub)"Repository: MFlowCode/MFC
Length of output: 286
🏁 Script executed:
cat -n .github/workflows/bench.yml | sed -n '37,79p'Repository: MFlowCode/MFC
Length of output: 2205
Scope polling to the latest Test Suite run and handle terminal non-success states.
The check-runs endpoint aggregates across reruns for the same SHA, so a prior failed run can block benchmarks even if a newer run succeeds. Additionally, skipped, cancelled, timed_out, or other non-success states for the Lint Gate job will currently hang until timeout instead of failing fast, as they fall through to the status logging logic and continue polling. Switch to the workflow runs and jobs endpoints to scope to the latest Test Suite run, and explicitly handle all terminal non-success states.
The job names are confirmed as correct: "Lint Gate" (line 11 of test.yml) and "Github" (line 64 of test.yml).
🛠️ Suggested update to poll the latest Test Suite run
- # Get the Test Suite workflow runs for this commit
- STATUS=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \
- --jq '.check_runs[] | select(.name == "Lint Gate") | .conclusion' | head -1)
+ # Get the latest Test Suite workflow run for this commit
+ RUN_ID=$(gh api repos/${{ github.repository }}/actions/workflows/test.yml/runs?head_sha=$SHA \
+ --jq '.workflow_runs | sort_by(.created_at) | last | .id')
+
+ if [ -z "$RUN_ID" ]; then
+ echo "No Test Suite run found yet..."
+ sleep 60
+ continue
+ fi
+
+ STATUS=$(gh api repos/${{ github.repository }}/actions/runs/$RUN_ID/jobs \
+ --jq '.jobs[] | select(.name == "Lint Gate") | .conclusion' | head -1)
- FAILED=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \
- --jq '[.check_runs[] | select(.name | startswith("Github")) | select(.conclusion == "failure")] | length')
+ FAILED=$(gh api repos/${{ github.repository }}/actions/runs/$RUN_ID/jobs \
+ --jq '[.jobs[] | select(.name | startswith("Github")) | select(.conclusion == "failure")] | length')
- PENDING=$(gh api repos/${{ github.repository }}/commits/$SHA/check-runs \
- --jq '[.check_runs[] | select(.name | startswith("Github")) | select(.conclusion == null)] | length')
+ PENDING=$(gh api repos/${{ github.repository }}/actions/runs/$RUN_ID/jobs \
+ --jq '[.jobs[] | select(.name | startswith("Github")) | select(.conclusion == null)] | length')
- elif [ "$STATUS" = "failure" ]; then
+ elif [ "$STATUS" = "failure" ] || [ "$STATUS" = "cancelled" ] || [ "$STATUS" = "timed_out" ] || [ "$STATUS" = "skipped" ]; then
echo "::error::Lint Gate failed. Benchmarks will not run."
exit 1🤖 Prompt for AI Agents
In @.github/workflows/bench.yml around lines 37 - 79, Replace the current
check-runs polling with logic that scopes to the latest Test Suite workflow run
and inspects that run's jobs: use the Actions workflow-runs endpoint to find the
most recent run for the Test Suite (scoped to the PR/head SHA stored in SHA),
then call the workflow run jobs endpoint to get job-level conclusions for that
run; look up the "Lint Gate" job conclusion and all jobs whose name starts with
"Github" from that run (not aggregated check-runs), and immediately fail if
"Lint Gate" is any terminal non-success (failure, cancelled, timed_out, skipped,
neutral, etc.), or if any "Github"-prefixed job has a terminal non-success
conclusion; only wait/poll while the latest run has non-terminal
in-progress/null conclusions, and loop by re-querying the latest workflow run id
each iteration so reruns don’t get blocked by earlier runs.
- Add .githooks/pre-commit that runs ./mfc.sh precheck before commits - Auto-install hook on first ./mfc.sh invocation (symlinks to .git/hooks/) - Hook only installs once; subsequent runs skip if already present - Developers can bypass with: git commit --no-verify Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="mfc.sh">
<violation number="1" location="mfc.sh:14">
P2: The auto-install hook check only supports repositories where `.git` is a directory; in worktrees `.git` is a file, so the hook never installs and precheck won’t run on commit. Consider resolving the git dir via `git rev-parse` so it works in worktrees/submodules too.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if [ -d "$(pwd)/.git" ] && [ ! -e "$(pwd)/.git/hooks/pre-commit" ] && [ -f "$(pwd)/.githooks/pre-commit" ]; then | ||
| ln -sf "$(pwd)/.githooks/pre-commit" "$(pwd)/.git/hooks/pre-commit" | ||
| log "Installed git pre-commit hook (runs$MAGENTA ./mfc.sh precheck$COLOR_RESET before commits)." | ||
| fi |
There was a problem hiding this comment.
P2: The auto-install hook check only supports repositories where .git is a directory; in worktrees .git is a file, so the hook never installs and precheck won’t run on commit. Consider resolving the git dir via git rev-parse so it works in worktrees/submodules too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mfc.sh, line 14:
<comment>The auto-install hook check only supports repositories where `.git` is a directory; in worktrees `.git` is a file, so the hook never installs and precheck won’t run on commit. Consider resolving the git dir via `git rev-parse` so it works in worktrees/submodules too.</comment>
<file context>
@@ -10,6 +10,12 @@ fi
. "$(pwd)/toolchain/util.sh"
+# Auto-install git pre-commit hook (once, silently)
+if [ -d "$(pwd)/.git" ] && [ ! -e "$(pwd)/.git/hooks/pre-commit" ] && [ -f "$(pwd)/.githooks/pre-commit" ]; then
+ ln -sf "$(pwd)/.githooks/pre-commit" "$(pwd)/.git/hooks/pre-commit"
+ log "Installed git pre-commit hook (runs$MAGENTA ./mfc.sh precheck$COLOR_RESET before commits)."
</file context>
| if [ -d "$(pwd)/.git" ] && [ ! -e "$(pwd)/.git/hooks/pre-commit" ] && [ -f "$(pwd)/.githooks/pre-commit" ]; then | |
| ln -sf "$(pwd)/.githooks/pre-commit" "$(pwd)/.git/hooks/pre-commit" | |
| log "Installed git pre-commit hook (runs$MAGENTA ./mfc.sh precheck$COLOR_RESET before commits)." | |
| fi | |
| git_dir="$(git rev-parse --git-dir 2>/dev/null)" | |
| hooks_dir="$(git rev-parse --git-path hooks 2>/dev/null)" | |
| if [ -n "$hooks_dir" ] && [ -d "$git_dir" ] && [ ! -e "$hooks_dir/pre-commit" ] && [ -f "$(pwd)/.githooks/pre-commit" ]; then | |
| ln -sf "$(pwd)/.githooks/pre-commit" "$hooks_dir/pre-commit" | |
| log "Installed git pre-commit hook (runs$MAGENTA ./mfc.sh precheck$COLOR_RESET before commits)." | |
| fi |
Auto-detect available CPUs for parallel formatting: - Linux: nproc - macOS: sysctl -n hw.ncpu - Fallback: 4 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Keep both: pre-commit hook auto-install and 'Starting...' log message Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mfc.sh`:
- Around line 65-68: The precheck branch in mfc.sh currently calls precheck.sh
with unquoted $@ which can split arguments containing spaces; update the
invocation in the elif block that sources toolchain/bootstrap/precheck.sh to use
"$@" (preserve argument boundaries) so the call is .
"$(pwd)/toolchain/bootstrap/precheck.sh" "$@"; also consider applying the same
change to the similar invocations for lint, format and spelling for consistency.
| elif [ "$1" '==' "precheck" ]; then | ||
| . "$(pwd)/toolchain/bootstrap/python.sh" | ||
|
|
||
| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 |
There was a problem hiding this comment.
Quote $@ to preserve argument boundaries.
The unquoted $@ on line 68 will re-split arguments containing spaces (e.g., ./mfc.sh precheck -j "4 cores" would incorrectly become three separate arguments). While this follows the existing pattern used by lint, format, and spelling commands, it's still a latent bug.
Proposed fix
elif [ "$1" '==' "precheck" ]; then
. "$(pwd)/toolchain/bootstrap/python.sh"
- shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0
+ shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" "$@"; exit 0
fiNote: Consider applying this fix to the other commands (
lint,format,spelling) for consistency, though that's outside the scope of this PR.
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 68-68: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
In `@mfc.sh` around lines 65 - 68, The precheck branch in mfc.sh currently calls
precheck.sh with unquoted $@ which can split arguments containing spaces; update
the invocation in the elif block that sources toolchain/bootstrap/precheck.sh to
use "$@" (preserve argument boundaries) so the call is .
"$(pwd)/toolchain/bootstrap/precheck.sh" "$@"; also consider applying the same
change to the similar invocations for lint, format and spelling for consistency.
Register precheck in commands.py so it appears in: - Shell tab completion - CLI documentation - ./mfc.sh precheck --help Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/mfc/cli/commands.py`:
- Around line 758-765: The Argument entry for "jobs" is missing type and default
so argparse will treat values as strings and return None when omitted; update
the Argument(...) for name="jobs" to include type=int and default=1 (and keep
short="j", help, metavar), or replace the explicit argument with
include_common=["jobs"] to reuse the COMMON_JOBS definition used elsewhere;
ensure you modify the Argument(...) block that defines "jobs" or swap it to
include_common so the parser receives an integer and a default.
| arguments=[ | ||
| Argument( | ||
| name="jobs", | ||
| short="j", | ||
| help="Number of parallel jobs for formatting.", | ||
| metavar="N", | ||
| ), | ||
| ], |
There was a problem hiding this comment.
Missing type and default for jobs argument.
The jobs argument lacks type=int and default=1, which other job-related arguments define (see COMMON_JOBS at lines 71-78). Without type=int, argparse will treat the value as a string rather than an integer. Without a default, the argument will be None when unspecified.
Consider either adding the missing attributes or using include_common=["jobs"] to reuse the common definition.
Proposed fix
PRECHECK_COMMAND = Command(
name="precheck",
help="Run CI lint checks locally before committing.",
description="Run the same fast checks that CI runs before expensive tests start. Use this locally before pushing to catch issues early.",
arguments=[
Argument(
name="jobs",
short="j",
help="Number of parallel jobs for formatting.",
+ type=int,
+ default=1,
metavar="N",
),
],Alternatively, use the common set:
PRECHECK_COMMAND = Command(
name="precheck",
help="Run CI lint checks locally before committing.",
description="Run the same fast checks that CI runs before expensive tests start. Use this locally before pushing to catch issues early.",
+ include_common=["jobs"],
- arguments=[
- Argument(
- name="jobs",
- short="j",
- help="Number of parallel jobs for formatting.",
- metavar="N",
- ),
- ],
+ arguments=[],📝 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.
| arguments=[ | |
| Argument( | |
| name="jobs", | |
| short="j", | |
| help="Number of parallel jobs for formatting.", | |
| metavar="N", | |
| ), | |
| ], | |
| arguments=[ | |
| Argument( | |
| name="jobs", | |
| short="j", | |
| help="Number of parallel jobs for formatting.", | |
| type=int, | |
| default=1, | |
| metavar="N", | |
| ), | |
| ], |
🤖 Prompt for AI Agents
In `@toolchain/mfc/cli/commands.py` around lines 758 - 765, The Argument entry for
"jobs" is missing type and default so argparse will treat values as strings and
return None when omitted; update the Argument(...) for name="jobs" to include
type=int and default=1 (and keep short="j", help, metavar), or replace the
explicit argument with include_common=["jobs"] to reuse the COMMON_JOBS
definition used elsewhere; ensure you modify the Argument(...) block that
defines "jobs" or swap it to include_common so the parser receives an integer
and a default.
When completion scripts are auto-regenerated, also update the installed completions at ~/.local/share/mfc/completions/ if they exist. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When installed shell completions are auto-updated, print a message with the appropriate source command for the user's detected shell. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously, installed completions only updated when source files changed and regeneration occurred. Now we also check if the installed completions are older than the generated ones (e.g., after git pull brings new pre-generated completions). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@toolchain/main.py`:
- Around line 35-48: The copy operations that update completions in install_dir
are unguarded and can raise OSError on read-only installs; wrap the shutil.copy2
calls inside try/except and make them best-effort: for each copy (for mfc.bash
and _mfc) catch OSError/PermissionError (or Exception) and on failure log a
non-fatal warning via cons.print that the completion update failed and continue
startup; apply the same try/except approach to the identical copy block around
lines 62-67 so neither __ensure_generated_files nor CLI startup aborts due to
permission issues.
toolchain/main.py
Outdated
| install_dir = Path.home() / ".local" / "share" / "mfc" / "completions" | ||
| if install_dir.exists(): | ||
| bash_updated = (install_dir / "mfc.bash").exists() | ||
| zsh_updated = (install_dir / "_mfc").exists() | ||
| if bash_updated: | ||
| shutil.copy2(completions_dir / "mfc.bash", install_dir / "mfc.bash") | ||
| if zsh_updated: | ||
| shutil.copy2(completions_dir / "_mfc", install_dir / "_mfc") | ||
| if bash_updated or zsh_updated: | ||
| # Detect user's shell and show appropriate source command | ||
| if "zsh" in os.environ.get("SHELL", "") and zsh_updated: | ||
| cons.print(f"[dim]Tab completions updated. Run: source {install_dir}/_mfc[/dim]") | ||
| elif bash_updated: | ||
| cons.print(f"[dim]Tab completions updated. Run: source {install_dir}/mfc.bash[/dim]") |
There was a problem hiding this comment.
Guard completion copies against permission errors to avoid blocking CLI startup.
__ensure_generated_files() runs on every invocation, so a read‑only ~/.local/share/mfc/completions will raise OSError and abort the command. Make the copy best‑effort and warn instead.
🛠️ Suggested fix (best‑effort copy with warning)
@@
- if bash_updated:
- shutil.copy2(completions_dir / "mfc.bash", install_dir / "mfc.bash")
- if zsh_updated:
- shutil.copy2(completions_dir / "_mfc", install_dir / "_mfc")
+ try:
+ if bash_updated:
+ shutil.copy2(completions_dir / "mfc.bash", install_dir / "mfc.bash")
+ if zsh_updated:
+ shutil.copy2(completions_dir / "_mfc", install_dir / "_mfc")
+ except OSError as exc:
+ cons.print(
+ f"[dim]Warning: couldn't update installed completions in {install_dir}: {exc}[/dim]"
+ )
@@
- if src.exists() and dst.exists() and os.path.getmtime(src) > os.path.getmtime(dst):
- shutil.copy2(src, dst)
- updated.append(("bash" if src_name == "mfc.bash" else "zsh", dst))
+ if src.exists() and dst.exists() and os.path.getmtime(src) > os.path.getmtime(dst):
+ try:
+ shutil.copy2(src, dst)
+ except OSError as exc:
+ cons.print(
+ f"[dim]Warning: couldn't update installed completions in {install_dir}: {exc}[/dim]"
+ )
+ continue
+ updated.append(("bash" if src_name == "mfc.bash" else "zsh", dst))Also applies to: 62-67
🤖 Prompt for AI Agents
In `@toolchain/main.py` around lines 35 - 48, The copy operations that update
completions in install_dir are unguarded and can raise OSError on read-only
installs; wrap the shutil.copy2 calls inside try/except and make them
best-effort: for each copy (for mfc.bash and _mfc) catch OSError/PermissionError
(or Exception) and on failure log a non-fatal warning via cons.print that the
completion update failed and continue startup; apply the same try/except
approach to the identical copy block around lines 62-67 so neither
__ensure_generated_files nor CLI startup aborts due to permission issues.
- Remove -o bashdefault from bash complete command to prevent falling back to directory completion when no matches found - Add explicit : (no-op) for zsh commands without arguments to prevent default file/directory completion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Auto-install completions on first mfc.sh run (via main.py) - Add -o filenames back to bash complete (needed for file completion) - Keep -o bashdefault removed to prevent directory fallback - Simplify code by having __update_installed_completions handle both install and update cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move completion auto-install to mfc.sh so it runs for ALL commands including help, precheck, etc. This ensures completions are always set up on first run. - Install completion files to ~/.local/share/mfc/completions/ - Add source line to .bashrc or fpath to .zshrc - Tell user to restart shell or source the file - main.py now only handles updates when generated files change Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
| # Add to shell rc file based on current shell | ||
| if [[ "$SHELL" == *"zsh"* ]]; then | ||
| RC_FILE="$HOME/.zshrc" | ||
| RC_LINE="fpath=(\"$COMPLETION_DIR\" \$fpath)" |
There was a problem hiding this comment.
Suggestion: For zsh, the completion directory is added to fpath but compinit is not rerun, so on a typical .zshrc where compinit is called earlier in the file the newly added directory will not actually be used for completions despite the log message claiming they are installed. [logic error]
Severity Level: Major ⚠️
- ⚠️ Zsh tab completions not active after installation.
- ⚠️ Users misled by completion-installed success message.| RC_LINE="fpath=(\"$COMPLETION_DIR\" \$fpath)" | |
| RC_LINE="fpath=(\"$COMPLETION_DIR\" \$fpath); autoload -Uz compinit; compinit" |
Steps of Reproduction ✅
1. Run ./mfc.sh from the repo root on a system without the completions directory; the
script creates completions and sets RC_LINE at mfc.sh:29 (PR hunk).
2. mfc.sh appends RC_LINE to the user's ~/.zshrc (code path around mfc.sh:37-41) but does
not re-run compinit in the current shell.
3. If the user's ~/.zshrc contains an earlier compinit invocation (common setup: "autoload
-Uz compinit; compinit"), the newly appended fpath entry will be processed only on next
shell start. The current shell will not pick up the new fpath, so completions are
effectively inactive.
4. The script logs "Installed tab completions. Restart shell or run: source ..." at
mfc.sh:43, which can mislead users into thinking sourcing the completion file alone is
sufficient; without running compinit after modifying fpath the completion will still not
be activated in zsh.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mfc.sh
**Line:** 29:29
**Comment:**
*Logic Error: For zsh, the completion directory is added to `fpath` but `compinit` is not rerun, so on a typical `.zshrc` where `compinit` is called earlier in the file the newly added directory will not actually be used for completions despite the log message claiming they are installed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| elif [ "$1" '==' "precheck" ]; then | ||
| . "$(pwd)/toolchain/bootstrap/python.sh" | ||
|
|
||
| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 |
There was a problem hiding this comment.
Suggestion: The arguments to the precheck subcommand are expanded as $@ instead of "$@", so any argument containing whitespace will be split into multiple arguments when passed to precheck.sh, causing its CLI parsing to misbehave for such inputs. [logic error]
Severity Level: Major ⚠️
- ⚠️ Local precheck CLI parsing can break with spaced arguments.
- ⚠️ Developers using filenames/paths with spaces affected.| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0 | |
| shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" "$@"; exit 0 |
Steps of Reproduction ✅
1. From the repository root, run the precheck command with an argument that contains a
space, e.g.:
./mfc.sh precheck "path/with space/file.txt"
(mfc.sh branch handling starts at mfc.sh:92 in the PR new hunk).
2. Execution enters the precheck branch at mfc.sh:92 and sources python bootstrap at
mfc.sh:93.
3. mfc.sh then executes (mfc.sh:95) the line:
shift; . "$(pwd)/toolchain/bootstrap/precheck.sh" $@; exit 0
Because $@ is unquoted, the single argument "path/with space/file.txt" is split into
two words by the shell.
4. The sourced script toolchain/bootstrap/precheck.sh receives two separate arguments
instead of one, causing its argument parsing to behave incorrectly (incorrect
option/filename boundaries) or to error. This reproduces the described misbehavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mfc.sh
**Line:** 95:95
**Comment:**
*Logic Error: The arguments to the `precheck` subcommand are expanded as `$@` instead of `"$@"`, so any argument containing whitespace will be split into multiple arguments when passed to `precheck.sh`, causing its CLI parsing to misbehave for such inputs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| #!/bin/bash | ||
| set -e | ||
| set -o pipefail | ||
|
|
There was a problem hiding this comment.
Suggestion: The script assumes it is always run from the repository root by hardcoding ./mfc.sh and ./src/* paths, so invoking precheck.sh from any other working directory (for example, running it directly from toolchain/bootstrap/) will cause it to fail to find mfc.sh or to skip source checks; computing the repo root from the script location and changing to it makes these relative paths reliable. [possible bug]
Severity Level: Major ⚠️
- ❌ Local precheck fails when run outside repository root.
- ⚠️ Formatting check errors due to missing ./mfc.sh file.
- ⚠️ Source lint searches miss files if cwd incorrect.| # Ensure we run from the repository root so relative paths like ./mfc.sh and ./src/* resolve correctly | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| REPO_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)" | |
| cd "$REPO_ROOT" |
Steps of Reproduction ✅
1. From a clone of the repository, change to the toolchain/bootstrap directory: cd
toolchain/bootstrap. This changes the cwd away from the repo root.
2. Execute the script directly: ./precheck.sh. The script later invokes ./mfc.sh in
toolchain/bootstrap/precheck.sh:67 (if ! ./mfc.sh format -j "$JOBS" > /dev/null 2>&1;
then). Because cwd=toolchain/bootstrap, ./mfc.sh does not exist and bash reports "No such
file or directory" from that line.
3. The script is run with set -e (toolchain/bootstrap/precheck.sh:2), so the missing
./mfc.sh call causes an immediate exit and the formatting check never runs.
4. Additionally, source checks use relative paths such as ./src/* in
toolchain/bootstrap/precheck.sh:107 (grep ... ./src/*) which will not point to repository
sources when cwd is not repo root, causing skipped or failing source lint checks. These
steps reproduce the issue using concrete file and line references.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/bootstrap/precheck.sh
**Line:** 4:4
**Comment:**
*Possible Bug: The script assumes it is always run from the repository root by hardcoding `./mfc.sh` and `./src/*` paths, so invoking `precheck.sh` from any other working directory (for example, running it directly from `toolchain/bootstrap/`) will cause it to fail to find `mfc.sh` or to skip source checks; computing the repo root from the script location and changing to it makes these relative paths reliable.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| shift | ||
| done | ||
|
|
||
| FAILED=0 |
There was a problem hiding this comment.
Suggestion: The script calls logging helpers log, ok, and error without defining them locally or sourcing any shared library, so running this script directly (or from an environment that does not export these functions) will immediately fail with "command not found" when the first log is executed; providing local fallbacks when these functions are absent makes the script robust in all invocation contexts. [possible bug]
Severity Level: Major ⚠️
- ❌ Local precheck command fails immediately when run directly.
- ⚠️ Developers cannot run prechecks without wrapper functions.
- ⚠️ CI-wrapper may mask this but direct execution breaks.| FAILED=0 | |
| # Provide basic logging helpers if they are not already defined by the caller | |
| if ! declare -F log >/dev/null 2>&1; then | |
| log() { echo "$@"; } | |
| fi | |
| if ! declare -F ok >/dev/null 2>&1; then | |
| ok() { echo "$@"; } | |
| fi | |
| if ! declare -F error >/dev/null 2>&1; then | |
| error() { echo "$@" >&2; } | |
| fi | |
Steps of Reproduction ✅
1. Open the file toolchain/bootstrap/precheck.sh and inspect for logging helpers: there
are no definitions for log/ok/error in this file (verify entire file content shown in PR).
2. Run the script directly from the repository root: bash toolchain/bootstrap/precheck.sh.
Execution reaches the first logging call on toolchain/bootstrap/precheck.sh:60 where the
script runs:
log "Running$MAGENTA precheck$COLOR_RESET (same checks as CI lint-gate)..."
3. Because log is not defined, bash prints "toolchain/bootstrap/precheck.sh: line 60: log:
command not found" and, due to set -e at toolchain/bootstrap/precheck.sh:2, the script
immediately exits with non-zero status.
4. Confirm by searching file for other uses of ok/error (e.g.,
toolchain/bootstrap/precheck.sh:80, :87, :96, :125, :133) — these will not be reached if
the first log call fails. This reproduces the failure with concrete file:line references.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/bootstrap/precheck.sh
**Line:** 58:58
**Comment:**
*Possible Bug: The script calls logging helpers `log`, `ok`, and `error` without defining them locally or sourcing any shared library, so running this script directly (or from an environment that does not export these functions) will immediately fail with "command not found" when the first `log` is executed; providing local fallbacks when these functions are absent makes the script robust in all invocation contexts.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| name="jobs", | ||
| short="j", | ||
| help="Number of parallel jobs for formatting.", | ||
| metavar="N", |
There was a problem hiding this comment.
Suggestion: The jobs option for the new command is missing an explicit integer type and default value, unlike the shared COMMON_JOBS definition used elsewhere; this means -j will be parsed as a string with a default of None, which is inconsistent with other commands and can cause downstream code that expects an integer with a default concurrency of 1 to misbehave or fail. [type error]
Severity Level: Major ⚠️
- ⚠️ CLI precheck parsing inconsistent with other commands.
- ⚠️ Local ./mfc.sh precheck uses different jobs semantics.
- ⚠️ Developers may receive confusing concurrency behavior.
- ⚠️ Tooling that introspects schema sees inconsistent types.| metavar="N", | |
| type=int, | |
| default=1, |
Steps of Reproduction ✅
1. Open a Python REPL in the repository root and import the CLI schema:
from toolchain.mfc.cli.commands import MFC_CLI_SCHEMA
(MFC_CLI_SCHEMA is defined in toolchain/mfc/cli/commands.py; PRECHECK_COMMAND is added
at commands.py:754-773).
2. Resolve the 'precheck' command via the schema helper (implementation at
toolchain/mfc/cli/schema.py:158-204):
cmd = MFC_CLI_SCHEMA.get_command("precheck")
This returns the Command object that was added in commands.py:754-773.
3. Inspect the argument definition (Argument dataclass is declared at
toolchain/mfc/cli/schema.py:44-81):
jobs_arg = cmd.arguments[0]
Observe jobs_arg.type is None and jobs_arg.default is None (i.e., no explicit
type/default).
4. Contrast with the shared COMMON_JOBS definition in the same file (COMMON_JOBS sets
Argument.type=int and default=1):
from toolchain.mfc.cli.commands import COMMON_JOBS
assert COMMON_JOBS.arguments[0].type is int and COMMON_JOBS.arguments[0].default == 1
The mismatch is concrete: PRECHECK_COMMAND.jobs lacks type/default while COMMON_JOBS
uses type=int default=1.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/cli/commands.py
**Line:** 763:763
**Comment:**
*Type Error: The `jobs` option for the new command is missing an explicit integer type and default value, unlike the shared `COMMON_JOBS` definition used elsewhere; this means `-j` will be parsed as a string with a default of `None`, which is inconsistent with other commands and can cause downstream code that expects an integer with a default concurrency of 1 to misbehave or fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
- -v/-vv/-vvv: output verbosity levels - --debug: build with debug compiler flags (for MFC Fortran code) - --debug-log/-d: Python toolchain debug logging (not MFC code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1122 +/- ##
=======================================
Coverage 44.03% 44.03%
=======================================
Files 70 70
Lines 20649 20649
Branches 2053 2053
=======================================
Hits 9093 9093
Misses 10368 10368
Partials 1188 1188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Avoid hogging resources on machines with many cores. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
size:L
|
CodeAnt AI Incremental review completed. |
1 participant
User description
User description
Summary
Motivation
Test plan
Generated with Claude Code
PR Type
Enhancement, Tests
Description
Add lint-gate CI job running fast checks before expensive tests
Add ./mfc.sh precheck command for local pre-push validation
Implement concurrency groups to cancel superseded CI runs
Lint-gate checks formatting, spelling, toolchain, and source code
Diagram Walkthrough
File Walkthrough
mfc.sh
Add precheck command entry pointmfc.sh
precheckcommand branch that sources precheck.sh scriptspellingandlintcommandsprecheck.sh
Implement local precheck validation scripttoolchain/bootstrap/precheck.sh
and junk code
test.yml
Add lint-gate job and concurrency configuration.github/workflows/test.yml
lint-gatejob running 5 fast checks before test matrixdouble precision intrinsics, and junk code patterns
githubandselfjobs depend onlint-gatecompletioncoverage.yml
Add concurrency group configuration.github/workflows/coverage.yml
cleanliness.yml
Add concurrency group configuration.github/workflows/cleanliness.yml
bench.yml
Add concurrency group configuration.github/workflows/bench.yml
CodeAnt-AI Description
Run CI lint gate locally and block expensive CI/benchmarks until fast checks pass
What Changed
Impact
✅ Fewer wasted HPC/benchmark runs✅ Shorter pre-push feedback loop (catch formatting/lint locally)✅ Fewer redundant CI runs on rapid pushes💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Chores
Documentation