Add harness documentation check - #50
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new documentation check, HarnessDocsCheck, that detects documented CI/quality-pipeline guidance (pass/partial/fail), registers it, adds tests (including Hypothesis variants), and updates the README counts to 32 checks. ChangesQuality Pipeline Documentation Check
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
tests/test_checks.py (1)
293-331: ⚡ Quick winAdd a Hypothesis test for regex-variant pipeline wording.
Given this check is regex-driven, add at least one property-based test that fuzzes casing/spacing/keyword variants to harden detection behavior beyond these three fixed fixtures.
As per coding guidelines
tests/test_*.py: Use property-based tests withhypothesisfor fuzz-like input generation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_checks.py` around lines 293 - 331, The tests lack a property-based Hypothesis test to fuzz regex variants for the harness pipeline wording; add a new test in tests/test_checks.py that uses Hypothesis strategies (e.g., text(), sampled_from(), one_of(), and regex-generated variants) to generate casing/spacing/keyword variants of phrases like "CI pipeline", "quality pipeline", "ci-pipeline", etc., build the repo context via _build_context with the generated doc content, run HarnessDocsCheck().run(context), and assert that expected detection occurs (passed/full score for strong matches, at least partial pass for contributing-only variants); reference HarnessDocsCheck and _build_context so the test exercises the same detection logic used by existing tests and follows the project's test style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 32-35: The example score table in README.md is inconsistent: the
"Architectural Documentation" category lists 6 checks but the example row shows
"3/5"; update that example row to use the correct denominator "3/6". Locate the
example score table row for "Architectural Documentation" in README.md and
change the displayed score from "3/5" to "3/6" so it matches the listed check
count.
---
Nitpick comments:
In `@tests/test_checks.py`:
- Around line 293-331: The tests lack a property-based Hypothesis test to fuzz
regex variants for the harness pipeline wording; add a new test in
tests/test_checks.py that uses Hypothesis strategies (e.g., text(),
sampled_from(), one_of(), and regex-generated variants) to generate
casing/spacing/keyword variants of phrases like "CI pipeline", "quality
pipeline", "ci-pipeline", etc., build the repo context via _build_context with
the generated doc content, run HarnessDocsCheck().run(context), and assert that
expected detection occurs (passed/full score for strong matches, at least
partial pass for contributing-only variants); reference HarnessDocsCheck and
_build_context so the test exercises the same detection logic used by existing
tests and follows the project's test style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9baed599-6a73-4541-a93f-4fba484cb883
📒 Files selected for processing (3)
README.mdsrc/ai_harness_scorecard/checks/documentation.pytests/test_checks.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_checks.py (1)
294-295:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign new test names with the required pass/fail naming convention.
The new methods should follow
test_<check_id>_pass/test_<check_id>_failfor consistency and discoverability.As per coding guidelines, "Name test functions as
test_<check_id>_passortest_<check_id>_fail".Also applies to: 314-315, 323-324, 346-347
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_checks.py` around lines 294 - 295, The test function names don't follow the required test_<check_id>_pass/fail convention; rename the new tests in tests/test_checks.py so they match that pattern (e.g., change test_harness_docs_pass_with_ci_pipeline_docs to test_harness_docs_pass) and likewise rename the other three newly added tests to test_<appropriate_check_id>_pass or test_<appropriate_check_id>_fail as applicable, and update any references/imports or pytest markers that use the old names (look for the HarnessDocsCheck-related tests and the three other new tests mentioned in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_checks.py`:
- Around line 358-361: The test is using TemporaryDirectory directly; change it
to use the pytest tmp_path fixture instead by adding tmp_path to the test
function signature and replacing the TemporaryDirectory() block with a Path from
tmp_path (e.g., tmp_path / "repo" or similar) passed into _build_context; update
any uses of Path(tmp_dir) to use that tmp_path-derived Path and remove the
TemporaryDirectory import/usage. Target symbols: the test function that calls
_build_context and the _build_context call itself.
---
Outside diff comments:
In `@tests/test_checks.py`:
- Around line 294-295: The test function names don't follow the required
test_<check_id>_pass/fail convention; rename the new tests in
tests/test_checks.py so they match that pattern (e.g., change
test_harness_docs_pass_with_ci_pipeline_docs to test_harness_docs_pass) and
likewise rename the other three newly added tests to
test_<appropriate_check_id>_pass or test_<appropriate_check_id>_fail as
applicable, and update any references/imports or pytest markers that use the old
names (look for the HarnessDocsCheck-related tests and the three other new tests
mentioned in the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5a5f069-e01a-46b6-a446-b25fa015b926
📒 Files selected for processing (2)
README.mdtests/test_checks.py
✅ Files skipped from review due to trivial changes (1)
- README.md
puneetdixit200
changed the title
markmishaev76
left a comment
markmishaev76
left a comment
There was a problem hiding this comment.
Thanks for this, and sorry it sat so long. The check is a good idea and the implementation is clean. Reviewed it locally on the PR head; one substantive concern about how loosely it credits, plus a couple of smaller notes.
Verified locally, because the CI workflows never ran on this PR (only CodeRabbit and GitGuardian reported, since Actions needs maintainer approval for a first-time contributor). On the PR head: 112 tests pass, ruff check and ruff format --check clean, mypy --strict clean across 18 source files. The branch is also up to date with main, so no rebase needed. I will approve the workflow run so the real suite reports too.
The category-scoring interaction is correct: CategoryScore.max_points sums max_points dynamically and the 20% weight comes from CATEGORY_CONFIG, so Architectural Docs going from 20 to 22 points dilutes within the category without touching cross-category weighting. The README table update to 3/6 is right.
Main concern: the fallback branches credit almost any repository
The check has three ways to score, and the lower two are loose enough that most repos will get points without documenting anything. Probed against the branch:
| repository content | score | evidence |
|---|---|---|
docs/setup.md containing only "We use pre-commit locally, optional." |
2.0/2.0 | Quality pipeline documented in docs/setup.md |
CI file whose only comment is # run the test suite |
1.0/2.0 | CI config comments mention quality checks |
CI file whose only comment is # workflow owner: platform |
1.0/2.0 | CI config comments mention quality checks |
CONTRIBUTING.md containing only # Contributing |
1.0/2.0 | Found CONTRIBUTING.md, but no documented quality pipeline |
Two specific causes:
-
\bpre-commit\bsits inPIPELINE_PATTERNSalongside genuinely strong signals likehow\s+to\s+add\s+(a\s+)?(new\s+)?(check|quality\s+gate|ci\s+job). A passing mention of the word earns full marks, which is a much weaker signal than the others in that list. -
COMMENTED_CI_PATTERNmatches^\s*#.*\b(ci|quality|check|lint|test|type|gate|workflow)\bagainstci_raw_content(). Withci,test,typeandworkflowin that alternation, virtually every CI file in existence has a comment that matches, including the wordworkflowappearing in an unrelated comment. That is close to free half credit.
This matters more than usual for this project because the whole value of the scorecard is discriminating between repos that have the safeguard and repos that do not. A check that awards ≥1.0/2.0 almost universally adds points without adding signal. Suggestions: drop bare pre-commit from the full-credit list, or demote it to the partial tier; and tighten the CI-comment alternation to multi-word phrases (quality gate, ci stage, pre-commit hook) rather than single common words.
Related: DOCUMENTATION_FILES uses docs/*.md, and because has_file/search_any_file use fnmatch on the full relative path, * also matches /. So docs/*.md recursively matches docs/adr/0002-additive-scoring-model.md and deeper. That is probably what you want, but it widens the false-positive surface for the loose patterns above, so it is worth being deliberate about.
Smaller notes
No test covers the COMMENTED_CI_PATTERN branch. The tests cover the full-credit path, the CONTRIBUTING.md partial path, the fail path, and the pattern variants, but nothing constructs a repo with only a CI config. That is the loosest branch in the check, so it is the one most worth pinning down.
check_id naming. documentation.harness_docs follows AGENTS.md, but all 31 existing checks use bare snake_case (architecture_doc, agent_instructions, adr_presence). Since check_id is emitted by the JSON reporter, this introduces one dotted ID into otherwise-flat output. The dotted form is the documented one so I would rather keep it and migrate the others separately, but flagging it so it is a deliberate choice. #52 makes the same call with testing.code_duplication.
Test import style. The tests import HarnessDocsCheck inside each test function. Everything else in tests/test_checks.py imports at module level; worth matching.
Merge coordination
#52 also adds a check and also rewrites the README from "31 checks" to "32 checks". Whichever merges second needs to say 33 and keep both category rows updated.
Happy to merge once the credit thresholds are tightened, or to take that on myself in a follow-up if you would rather land the check as-is; let me know which you prefer.
|
Closing and immediately reopening to trigger the CI workflows, which never ran on this PR (fork PR, zero recorded runs). Not a rejection. |
|
CI is now green on all 9 checks (lint, typecheck, test, security, audit, docs, self-assess, plus CodeRabbit and GitGuardian), which matches what I saw locally. The review feedback above is the only thing outstanding. |
2 participants
What does this PR do?
Adds the
documentation.harness_docscheck proposed in #33. The check awards full credit when CONTRIBUTING/docs describe CI stages, quality gates, pre-commit, development workflow, or how to add a quality check; partial credit is available for a CONTRIBUTING.md without pipeline detail or CI comments documenting checks.Closes #33.
Type of change
Checklist
ruff checkandruff formatpassmypy src/passes31/32checks pass, grade A 92.0/100)Testing
.venv/bin/python -m pytest tests/test_checks.py -q -k harness_docs.venv/bin/python -m pytest tests/test_checks.py -q.venv/bin/python -m pytest -q.venv/bin/ruff check src/ tests/.venv/bin/ruff format --check src/ tests/.venv/bin/mypy src/.venv/bin/bandit -r src/ -c pyproject.toml.venv/bin/pip-audit.venv/bin/ai-harness-scorecard assess .git diff --checkAI Usage
This PR was prepared with AI assistance. I reviewed the implementation and validated it with the commands listed above.
Summary by CodeRabbit
New Features
Documentation
Tests