Add governance report pack with validator, tests, Makefile, pre-commit and CI workflow#65
Merged
OneFineStarstuff merged 2 commits intoApr 27, 2026
Conversation
|
The files' contents are under analysis for test generation. |
|
Review changes with Changed Files
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
View changes in DiffLens |
Reviewer's GuideAdds a structured governance report pack with a machine-validated manifest, plus a Python validator wired into tests, Makefile targets, pre-commit, and a GitHub Actions workflow to enforce report integrity locally and in CI. Sequence diagram for local and CI governance report validationsequenceDiagram
actor Developer
participant Git as git
participant PreCommit as pre_commit_hooks
participant Make as make
participant Validator as validate_governance_reports_py
participant Tests as unittest_suite
participant CI as github_actions
Developer->>Git: git commit
Git->>PreCommit: run pre-commit (pre-commit stage)
PreCommit->>Make: make governance-validate
Make->>Validator: python3 tools/validate_governance_reports.py
Validator-->>Make: exit code (0/1)
Make-->>PreCommit: propagate status
PreCommit-->>Git: allow or block commit
Developer->>Git: git push
Git->>PreCommit: run pre-push hooks
PreCommit->>Make: make governance-check
Make->>Tests: python3 -m unittest discover tool_tests
Tests-->>Make: test results
Make->>Validator: python3 tools/validate_governance_reports.py
Validator-->>Make: validation status
Make->>Validator: python3 tools/validate_governance_reports.py --json
Validator-->>Make: JSON payload {status, counts}
Make-->>PreCommit: propagate status
PreCommit-->>Git: allow or block push
Developer->>CI: open PR / push
CI->>CI: Governance Reports Validation workflow
CI->>PreCommit: pre-commit run --all-files
PreCommit->>Make: make governance-validate (pre-commit stage)
Make->>Validator: validate_governance_reports.py
Validator-->>Make: status
Make-->>PreCommit: status
PreCommit-->>CI: status
CI->>Make: make governance-check
Make->>Tests: run tool_tests
Tests-->>Make: results
Make->>Validator: validate_governance_reports.py (CLI + JSON check)
Validator-->>Make: status
Make-->>CI: final status
Flow diagram for validate_governance_reports.py logicflowchart TD
Start([start]) --> ParseArgs[parse_arguments]
ParseArgs --> CollectErrors[collect_validation_errors]
subgraph collect_validation_errors
CE1[for_each_report_in_REPORT_RULES]
CE1 --> VF[validate_file]
VF --> CE2[accumulate_errors_and_count]
CE2 --> VR[validate_readme_index]
VR --> VM[validate_manifest]
VM --> VMS[validate_manifest_schema]
end
CollectErrors --> HasErrors{all_errors_empty?}
HasErrors -- no --> HandleFail[prepare_failure_output]
HandleFail --> FailJson{--json_flag?}
FailJson -- yes --> PrintFailJson[print JSON status:failed,error_count,errors]
FailJson -- no --> PrintFailText[print_human_readable_errors]
PrintFailJson --> ExitFail[exit_code_1]
PrintFailText --> ExitFail
HasErrors -- yes --> HandleSuccess[prepare_success_output]
HandleSuccess --> SuccJson{--json_flag?}
SuccJson -- yes --> PrintSuccJson[print JSON status:passed,counts]
SuccJson -- no --> PrintSuccText[print summary line]
PrintSuccJson --> ExitOK[exit_code_0]
PrintSuccText --> ExitOK
subgraph validate_file
VFCheck1[check_required_tags]
VFCheck2[check_single_title_abstract_content_blocks]
VFCheck3[check_title_non_empty_len>=10]
VFCheck4[check_required_headings_present]
end
subgraph validate_manifest
VMRead[read_manifest_JSON]
VMSchema[maybe_read_schema_root_and_item_required_sets]
VMFields[check_root_fields_version_report_pack_reports]
VMReportsIter[iterate_reports_list]
VMReportChecks[check_path_audience_required_types_and_existence]
VMConsistency[check_manifest_vs_expected_report_paths]
end
subgraph validate_manifest_schema
SMRead[read_schema_JSON]
SMRootReq[check_root_required_has_version_report_pack_reports]
SMProps[check_reports_properties_items_required_path_audience_required]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Review these changes at https://app.gitnotebooks.com/OneFineStarstuff/OneFineStarstuff.github.io/pull/65 |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ 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 |
|
View changes in DiffLens |
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
Contributor
|
Failed to generate code suggestions for PR |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Compatibility | 14 medium |
| Documentation | 37 minor |
| ErrorProne | 15 high |
| CodeStyle | 30 minor |
| Complexity | 4 medium |
🟢 Metrics 117 complexity · 2 duplication
Metric Results Complexity 117 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The
governance-validate-json-checkMakefile target hardcodes/tmpas the output directory, which can be brittle on non-POSIX environments; consider usingtempfile(or at least${TMPDIR}) via a small Python helper to make the check portable. - The
validate_filelogic relies on simple substring counting for tags and headings, which can be tripped by commented-out sections or duplicated headings; you might want to tighten this by parsing the wrappers and headings more structurally (e.g., regex over specific regions) to reduce false positives as the documents evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `governance-validate-json-check` Makefile target hardcodes `/tmp` as the output directory, which can be brittle on non-POSIX environments; consider using `tempfile` (or at least `${TMPDIR}`) via a small Python helper to make the check portable.
- The `validate_file` logic relies on simple substring counting for tags and headings, which can be tripped by commented-out sections or duplicated headings; you might want to tighten this by parsing the wrappers and headings more structurally (e.g., regex over specific regions) to reduce false positives as the documents evolve.
## Individual Comments
### Comment 1
<location path="tools/validate_governance_reports.py" line_range="149-151" />
<code_context>
+ if "report_pack" in root_required and (not isinstance(report_pack, str) or not report_pack.strip()):
+ errors.append(f"{path}: 'report_pack' must be a non-empty string")
+
+ reports = data.get("reports")
+ if not isinstance(reports, list):
+ return [f"{path}: 'reports' must be a list"]
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning immediately on non-list `reports` discards earlier manifest errors
This early return drops any errors already collected (e.g., missing fields, invalid `version`/`report_pack`, schema errors). Instead of returning a new single-item list, append this message to `errors` and return `errors`, so callers see the complete set of issues.
```suggestion
report_pack = data.get("report_pack")
if "report_pack" in root_required and (not isinstance(report_pack, str) or not report_pack.strip()):
errors.append(f"{path}: 'report_pack' must be a non-empty string")
reports = data.get("reports")
if not isinstance(reports, list):
errors.append(f"{path}: 'reports' must be a list")
return errors
```
</issue_to_address>
### Comment 2
<location path="tool_tests/test_validate_governance_reports.py" line_range="245" />
<code_context>
+ self.assertTrue(any("'version' must be a non-empty string" in e for e in errors))
+ self.assertTrue(any("'report_pack' must be a non-empty string" in e for e in errors))
+
+ def test_validate_manifest_schema_accepts_valid_schema(self):
+ with tempfile.TemporaryDirectory() as tmpdir:
+ path = Path(tmpdir) / "schema.json"
</code_context>
<issue_to_address>
**suggestion (testing):** Extend validate_manifest_schema tests to cover malformed schema structures and type errors
Right now we only cover a valid schema and missing required fields, but `validate_manifest_schema` has several other guard paths that go untested: invalid/non-JSON input, non-dict root, non-list `required`, non-dict `properties`, missing or non-dict `reports`, non-dict `items`, and non-list `items.required`. Please add a few minimal tests that exercise each of these malformed-schema cases so those branches are covered and remain stable as the schema structure evolves.
Suggested implementation:
```python
def test_validate_manifest_schema_accepts_valid_schema(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version", "report_pack", "reports"],
"properties": {
"reports": {
"items": {
"required": ["path", "audience", "required"]
}
}
}
}
"""
)
errors = validate_manifest_schema(path)
self.assertEqual(errors, [])
def test_validate_manifest_schema_rejects_invalid_json(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
# Intentionally invalid JSON
path.write_text("{ this is not valid json }")
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("invalid JSON" in e or "JSON" in e for e in errors),
msg=f"Expected an invalid JSON error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_dict_root(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text("[]")
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("root" in e and "object" in e for e in errors),
msg=f"Expected a non-dict root error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_list_required(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": "version",
"properties": {}
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'required' must be a list" in e for e in errors),
msg=f"Expected 'required' must be a list error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_dict_properties(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version"],
"properties": []
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'properties' must be an object" in e or "'properties' must be a dict" in e for e in errors),
msg=f"Expected 'properties' must be a dict error, got: {errors}",
)
def test_validate_manifest_schema_rejects_missing_reports_property(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version", "report_pack", "reports"],
"properties": {}
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'reports' property" in e or "reports" in e for e in errors),
msg=f"Expected missing 'reports' property error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_dict_reports_property(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version", "report_pack", "reports"],
"properties": {
"reports": []
}
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'reports' must be an object" in e or "'reports' must be a dict" in e for e in errors),
msg=f"Expected 'reports' must be a dict error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_dict_items(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version", "report_pack", "reports"],
"properties": {
"reports": {
"items": []
}
}
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'items' must be an object" in e or "'items' must be a dict" in e for e in errors),
msg=f"Expected 'items' must be a dict error, got: {errors}",
)
def test_validate_manifest_schema_rejects_non_list_items_required(self):
with tempfile.TemporaryDirectory() as tmpdir:
path = Path(tmpdir) / "schema.json"
path.write_text(
"""
{
"required": ["version", "report_pack", "reports"],
"properties": {
"reports": {
"items": {
"required": "path"
}
}
}
}
"""
)
errors = validate_manifest_schema(path)
self.assertTrue(errors)
self.assertTrue(
any("'items.required' must be a list" in e or "items.required" in e for e in errors),
msg=f"Expected 'items.required' must be a list error, got: {errors}",
)
```
These tests assume `validate_manifest_schema(path)`:
1. Returns a list of human-readable error strings instead of raising exceptions.
2. Uses messages containing phrases like:
- "invalid JSON" for JSON parse errors
- "'required' must be a list"
- "'properties' must be an object" / "dict"
- "'reports' must be an object" / "dict"
- "'items' must be an object" / "dict"
- "'items.required' must be a list" or similar.
If the actual error messages differ, update the `any(... in e ...)` predicates to match the real strings (you can run the tests and adjust to the exact wording emitted by `validate_manifest_schema`). The structural aspects of the tests (what kind of malformed schema they feed in) should remain the same.
</issue_to_address>
### Comment 3
<location path="tool_tests/test_validate_governance_reports.py" line_range="94" />
<code_context>
+ self.assertEqual(len(errors), 1)
+ self.assertIn("missing file", errors[0])
+
+ def test_validate_readme_index_accepts_valid_readme(self):
+ with tempfile.TemporaryDirectory() as tmpdir:
+ path = Path(tmpdir) / "README_GOVERNANCE_REPORTS.md"
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for validate_readme_index when the README is missing
Please add a test that calls `validate_readme_index` with a non-existent README path and asserts that the `missing file: ...` error is returned. This will cover the missing-file branch and guard against regressions if the README is deleted or renamed.
Suggested implementation:
```python
def test_validate_file_rejects_missing_file(self):
with tempfile.TemporaryDirectory() as tmpdir:
missing = Path(tmpdir) / "missing.md"
errors = validate_file(missing, [])
self.assertEqual(len(errors), 1)
self.assertIn("missing file", errors[0])
def test_validate_readme_index_rejects_missing_readme(self):
with tempfile.TemporaryDirectory() as tmpdir:
missing_readme = Path(tmpdir) / "README_GOVERNANCE_REPORTS.md"
errors = validate_readme_index(missing_readme)
self.assertEqual(len(errors), 1)
self.assertIn("missing file", errors[0])
def test_validate_readme_index_accepts_valid_readme(self):
```
If `validate_readme_index` is not already imported into this test module, add it to the relevant import statement (likely alongside `validate_file`) at the top of `tool_tests/test_validate_governance_reports.py`.
</issue_to_address>
### Comment 4
<location path="tool_tests/test_validate_governance_reports.py" line_range="35" />
<code_context>
+
+
+class ValidateGovernanceReportsTests(unittest.TestCase):
+ def test_validate_file_accepts_valid_document(self):
+ with tempfile.TemporaryDirectory() as tmpdir:
+ path = Path(tmpdir) / "doc.md"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for title-length validation and other validate_file failure modes
Current `validate_file` tests cover missing tags, duplicate blocks, missing headings, and missing files, but they don’t exercise the title-length branch (`title is empty or too short`). Please add a test that writes a document with a `<title>` block containing an empty or very short string and asserts the `title is empty or too short` error. If straightforward, also add a case where all required tags are present but counts don’t match (e.g., two `<title>` and one `</title>`) to verify the "expected exactly one ..." errors are raised.
</issue_to_address>
### Comment 5
<location path="tool_tests/test_validate_governance_reports.py" line_range="334-339" />
<code_context>
result = subprocess.run(
[sys.executable, "tools/validate_governance_reports.py", "--json"],
check=True,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
❌ Deploy Preview for onefinestarstuff failed.
|
…nterprises Signed-off-by: 𝐎𝐧𝐞 𝐅𝐢𝐧𝐞 𝐒𝐭𝐚𝐫𝐬𝐭𝐮𝐟𝐟 <onefinestarstuff@gmail.com>
|
View changes in DiffLens |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
docs/reports/plus aREADME_GOVERNANCE_REPORTS.md, a machine-readablegovernance_reports_manifest.json, and its JSON Schema atdocs/schemas/governance_reports_manifest.schema.json.tools/validate_governance_reports.pythat checks required<title>,<abstract>,<content>wrappers, required section anchors per report, README index entries, manifest consistency, and schema requirements, and supports--jsonoutput.tool_tests/test_validate_governance_reports.pycovering file validation, README index checks, manifest and schema validations, and JSON output mode.Makefiletargets (governance-test,governance-validate,governance-validate-json,governance-validate-json-check,governance-check), a local pre-commit hook config.pre-commit-config.yamlto run the validator on commit/push, and a GitHub Actions workflow.github/workflows/governance-reports.ymlto run the full validation suite in CI.Testing
python3 -m unittest discover tool_tests, which executed the validator tests and succeeded.python3 tools/validate_governance_reports.py --json, which returned astatus: passedpayload for the added reports.make governance-check(runs tests + validator + JSON status check), which completed successfully in local verification and is wired into the CI workflow for automated runs.Codex Task
Summary by Sourcery
Introduce a machine-validated governance report pack with automated validation and CI integration.
New Features:
Enhancements:
Tests: