fix(scripts): repoint checks at the .adoc files that exist - #69
Conversation
The .md -> .adoc documentation migration moved these files but never updated
the scripts that READ them, so every check naming a .md has been operating on
a file that no longer exists.
Repointed: ABI-FFI-README.md->ABI-FFI-README.adoc
Three failure modes were in play across the estate, all fixed by the same
change:
* hard fail - 'check "X.md exists" "[ -f X.md ]"' can never pass
* wrong score - '[ -f X.md ] && ((doc_score++))' silently scores lower
* SILENT SKIP - 'if [ -f X.md ]; then ...greps... fi' skips the whole block,
so the checks inside never run and the gate reports success
by not checking at all
Human-readable labels are repointed too, so failure messages name the file that
is actually inspected. Where a script did 'git add ... X.md', that is fixed as
well - it would have failed at release time.
Only tokens whose .adoc twin exists in this repository were rewritten; anything
without a twin was left untouched for separate triage.
Found by an estate-wide sweep of 454 repos: 56 such checks across 18 repos.
Same defect class as hyperpolymath/Axiom.jl#82.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🪛 GitHub Check: SonarCloud Code Analysistests/validate_structure.sh[failure] 38-38: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 43-43: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 53-53: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 31-31: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 48-48: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 21-21: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. [failure] 58-58: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich. 🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe structural validation script now uses explicit checks for required files, directories, and key files. It validates ChangesStructure validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized change repoints validation scripts to existing documentation files; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches📝 Generate docstrings
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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR is currently not up to standards due to a high-severity logic issue in the validation script. While the update to the .adoc file extension aligns with the documentation migration, the implementation in tests/validate_structure.sh uses a brittle [ condition ] && pass || fail pattern. This construct is unsafe as it does not guarantee standard if-then-else behavior; if the 'pass' command fails, the 'fail' branch will execute regardless of the condition's result. This flaw should be addressed to ensure reliable CI checks.
Test suggestions
- Verify
validate_structure.shsucceeds whenABI-FFI-README.adocis present - Verify
validate_structure.shfails with an appropriate error message whenABI-FFI-README.adocis missing
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify `validate_structure.sh` succeeds when `ABI-FFI-README.adoc` is present
2. Verify `validate_structure.sh` fails with an appropriate error message when `ABI-FFI-README.adoc` is missing
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| [ -f LICENSE ] && pass "LICENSE present" || fail "LICENSE missing" | ||
| [ -f SECURITY.md ] && pass "SECURITY.md present" || fail "SECURITY.md missing" | ||
| [ -f ABI-FFI-README.md ] && pass "ABI-FFI-README.md present" || fail "ABI-FFI-README.md missing" | ||
| [ -f ABI-FFI-README.adoc ] && pass "ABI-FFI-README.adoc present" || fail "ABI-FFI-README.adoc missing" |
There was a problem hiding this comment.
🔴 HIGH RISK
The construct [ condition ] && pass || fail is not a safe substitute for an if-then-else statement. If the pass function returns a non-zero exit status, the fail branch will be executed even if the file exists. Replace this with a standard if-block for better reliability. Try running the following prompt in your IDE agent: > Refactor line 19 in tests/validate_structure.sh to use a proper if-then-else statement instead of the &&/|| shorthand to resolve ShellCheck SC2015.
| [ -f LICENSE ] && pass "LICENSE present" || fail "LICENSE missing" | ||
| [ -f SECURITY.md ] && pass "SECURITY.md present" || fail "SECURITY.md missing" | ||
| [ -f ABI-FFI-README.md ] && pass "ABI-FFI-README.md present" || fail "ABI-FFI-README.md missing" | ||
| [ -f ABI-FFI-README.adoc ] && pass "ABI-FFI-README.adoc present" || fail "ABI-FFI-README.adoc missing" |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Automated verification is needed to confirm the script correctly handles both the presence and absence of the new .adoc files.
Two defect classes, both verified empirically rather than inferred.
1. '[ cond ] && pass || fail' is NOT if/then/else.
If the 'pass' branch returns non-zero, the 'fail' branch ALSO runs --
even though the condition was true. Demonstrated:
pass() { echo ran; return 1; }
[ -n yes ] && pass || fail
-> BOTH pass() and fail() execute
Rewritten as explicit if/then/else.
2. '((var++))' dies under 'set -e' when the counter is 0.
Post-increment returns the OLD value as its exit status, so the first
increment of a zero counter exits 1 and 'set -e' terminates the
script. Demonstrated:
set -e; score=0; ((score++)); echo reached
-> script DIES before 'reached'; works fine from 1 onward
That is precisely the first-document case a compliance script hits on
every run. Rewritten as 'var=$((var + 1))'.
Both classes are the same underlying trap as the duplicate-branch bug
already fixed on asdf-tool-plugins#70 and developer-ecosystem#191:
shell shorthand that reads like control flow but is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
1 participant
The
.md→.adocdocumentation migration moved these files but never updated the scripts that read them, so every check naming a.mdhas been operating on a file that no longer exists.Repointed: ABI-FFI-README.md->ABI-FFI-README.adoc
Three failure modes were in play across the estate, all fixed by the same change:
check "X.md exists" "[ -f X.md ]"[ -f X.md ] && ((doc_score++))if [ -f X.md ]; then …greps… fiLabels are repointed too, so failure messages name the file actually inspected. Where a script did
git add … X.md, that is fixed as well — it would have failed at release time.Only tokens whose
.adoctwin exists here were rewritten; anything without a twin was left for separate triage.Found by an estate-wide sweep of 454 repos: 56 such checks across 18 repos. Same class as hyperpolymath/Axiom.jl#82.