fix(rsr-verify): accept .adoc — the docs exist, the check named .md - #194
fix(rsr-verify): accept .adoc — the docs exist, the check named .md#194hyperpolymath wants to merge 2 commits into
Conversation
rsr-verify.sh fails 7 documentation checks in a plugin whose documentation is COMPLETE. It checks docs/ARCHITECTURE.md, API_REFERENCE.md, FAQ.md, QUICKSTART.md, TROUBLESHOOTING.md, EXAMPLES.md and MIGRATION.md; the plugin ships all seven as .adoc, plus CITATIONS.adoc and COMPATIBILITY.adoc. The .md -> .adoc migration moved the documents and left the verifier naming the old extension, so seven RSR compliance checks have been failing on a compliant plugin. Fixed by accepting either extension - the same pattern the RSR template's own quality.yml uses for README, LICENSE and CONTRIBUTING. The check label drops the extension too, so a failure names the document rather than a filename that may legitimately be either form. Verified: 7/7 checks satisfied against the files actually on disk, 0 missing, shellcheck -S error reports 0 findings. ⚠ I first filed these as MISSING documentation in hyperpolymath/standards#653 and was about to author 14 replacement files. That was wrong: my classifier looked for the .adoc twin at the REPO ROOT, but these live at asdf-ghjk/docs/. Issue #653 is being corrected.
|
|
|
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. (3)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RSR verification script now accepts either Markdown or AsciiDoc files for seven documentation categories. It replaces Markdown-only file checks. ChangesDocumentation validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized change makes documentation checks accept either .md or .adoc files without changing documented content or broader runtime behavior. No actionable merge-blocking risk remains after 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
|
There was a problem hiding this comment.
Pull Request Overview
While Codacy identifies the PR as being up to standards, the functional review reveals high-severity logic bugs in rsr-verify.sh. The boolean expressions used to check for file existence and line counts (lines 64 and 106) suffer from operator precedence issues. In Bash, && and || have equal precedence and are evaluated left-to-right, meaning the current implementation will incorrectly fail if the .adoc variant is missing, even if a valid .md file is present.
Furthermore, an acceptance criteria gap was identified: the requirement for checks to pass with either file type is currently broken by this logic. A threshold typo in the CONTRIBUTING check also exists. These issues must be addressed before merging to ensure the script functions as intended.
About this PR
- The PR lacks automated test coverage for the bash script logic; verification was performed manually, which does not prevent future regressions in compliance enforcement.
2 comments outside of the diff
asdf-augmenters/asdf-ghjk/scripts/rsr-verify.sh
line 64🔴 HIGH RISK
The boolean logic here will fail the check ifREADME.adocis missing, even if a validREADME.mdexists. Use curly braces to group the logic correctly so that each format is checked independently.Suggested fix:
check "README is comprehensive (>100 lines)" "{ [[ -f README.md ]] && [[ \$(wc -l < README.md) -gt 100 ]]; } || { [[ -f README.adoc ]] && [[ \$(wc -l < README.adoc) -gt 100 ]]; }"
line 106🔴 HIGH RISK
This check contains a logic error and a threshold inconsistency. The current boolean structure incorrectly requires the second file check to pass if the first one succeeded, and the.adocthreshold should be50to match the.mdrequirement.Suggested fix:
check "CONTRIBUTING comprehensive (>50 lines)" "{ [[ -f CONTRIBUTING.md ]] && [[ \$(wc -l < CONTRIBUTING.md) -gt 50 ]]; } || { [[ -f CONTRIBUTING.adoc ]] && [[ \$(wc -l < CONTRIBUTING.adoc) -gt 50 ]]; }"
Test suggestions
- Verify check passes when a required document exists with a .md extension in the docs/ directory.
- Verify check passes when a required document exists with a .adoc extension in the docs/ directory.
- Verify check fails and displays the extension-agnostic label when a required document is missing both extensions.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify check passes when a required document exists with a .md extension in the docs/ directory.
2. Verify check passes when a required document exists with a .adoc extension in the docs/ directory.
3. Verify check fails and displays the extension-agnostic label when a required document is missing both extensions.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
rsr-verify.shfails 7 documentation checks in a plugin whose documentation is complete.Every document exists. The
.md→.adocmigration moved them and left the verifier naming the old extension, so 7 of the RSR compliance checks have been failing on a compliant plugin.Fix
Accept either extension — the same pattern the RSR template's own
quality.ymlalready uses for README, LICENSE and CONTRIBUTING:The check label drops the extension too, so a failure message names the document rather than a filename that may be either form.
Verified
shellcheck -S errorProvenance
Found by an estate-wide sweep of 5,111 scripts across 375 repos. Same defect class as the 56 unsatisfiable checks repointed across 18 repos earlier in this campaign (see hyperpolymath/Axiom.jl#82).
⚠ I initially filed these as missing documentation in hyperpolymath/standards#653 and was about to author 14 replacement files. That was wrong — my classifier looked for the
.adoctwin at the repo root, but these live atasdf-ghjk/docs/, so it reported no twin. Issue #653 is being corrected.