Skip to content

fix: add a test suite so the Best Practices badge can pass - #12

Merged
lusoris merged 6 commits into
mainfrom
fix/cii-badge-tests
Nov 8, 2026
Merged

lusoris merged 6 commits into
mainfrom
fix/cii-badge-tests

Conversation

@lusoris

@lusoris lusoris commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds an automated test suite and a test policy, so this repository meets the four OpenSSF Best Practices MUST criteria that block a passing badge. This works toward closing code-scanning alert 9 (Scorecard CII-Best-Practices, "badge detected: InProgress").

Why the alert is still open

Project 16160 on bestpractices.dev is in_progress at 94 % (passing level, read from projects/16160.json, last saved 2026-11-08 13:42 UTC). Four MUST criteria are Unmet in .bestpractices.json on main:

  • test
  • test_policy
  • tests_are_added
  • release_notes

Scorecard scores a passing badge 5, which closes the alert.

What changes

  • Test suite (tests/, Python unittest, 47 tests):
    • labels.yml matches the shape EndBug/label-sync reads (its src/index.ts at the pinned commit), gives every label a six-digit colour (GitHub REST docs), and repeats no name or alias.
    • .bestpractices.json gives every criterion a valid status and a justification.
    • Every relative link and image in the profile, the community health files and the decision records resolves. Links come from markdown-it-py's token stream, so inline links, reference definitions, angle-bracket targets and percent-encoded paths are read the way a CommonMark renderer reads them.
    • Each check has a passing case, planted-defect cases and boundary cases.
    • Every test and helper has a one-line docstring naming the case it covers. The pre-push docstrings gate (80 %, from .coderabbit.yaml) now reads 61 of 61.
  • Runner: make test runs the suite, and make verify-all already includes test. PyYAML and markdown-it-py are pinned with hashes in .config/tests/requirements.txt (uv, the same command as the REUSE requirements).
  • CI: a new Tests workflow (.github/workflows/test.yml) runs make test on pull requests, on main and in the merge queue. It follows reuse.yml: pinned checkout, the draft stop step, contents: read and a 10-minute timeout.
  • Required check: .github/rulesets/main.json lists make test as a required status check. The live ruleset praetor-main-protection (25275401) was updated from this file through the REST API before this pull request opened, so the audit sees no drift. The live ruleset keeps the settings it already held that are stricter than the file.
  • Test policy: a new ## Tests section in README.md. It goes there rather than in CONTRIBUTING.md, because that file is the organisation-wide default for every golusoris repository.
  • Best Practices answers (.bestpractices.json):
    • test, test_invocation, test_policy, tests_are_added and test_continuous_integration are now Met, each pointing at the file that proves it.
    • release_notes is now N/A. The repository publishes no releases (0 releases, 0 tags). GitHub serves its files from the default branch, and sync-labels.yml is called at @main and fetches labels.yml from main. That is the "continuously updated, deployed to one place" case for which the criterion allows N/A.
    • Two justifications that cited the old "test not applicable" recipe now describe the suite.
  • Generated context: .paperclip/harness.json, .paperclip/rules.md and the source hash in .standards.yaml are the praetorctl compile-context output for the new tree.

Evidence

  • make test: 47 tests, OK.
  • Mutation run: each check is switched off or weakened in a copy of tests/repo_checks.py (no deduplication, alt-text links, no escape check, no unquoting, no strip on names or aliases, no reference definitions, no images). All 8 mutants fail at least one test, and the unmutated copy passes.
  • make verify-all exits 0 on the branch head. All pre-commit and pre-push hooks passed, including audit, docstrings (100 %) and gate.
  • actionlint and zizmor --offline report nothing on test.yml, markdownlint-cli2 README.md reports 0 issues, reuse lint passes and ruff check tests/ is clean.

After merge

The badge level changes only after project 16160 is saved from the updated .bestpractices.json. The criteria the file does not answer yet (for example homepage_url, report_url and the crypto_* group, shown as unknown) are filled with evidence in a follow-up pull request. Scorecard's next run on main then reads the new level.

One pre-push warning predates this pull request and stays: scorecard.yml has no draft step. It does not fail the gate.

Summary by Sourcery

Establish automated repository validation and CI coverage to satisfy the remaining OpenSSF Best Practices testing criteria.

New Features:

  • Add a comprehensive automated test suite validating published labels, Best Practices answers, required checks, test policy, and Markdown links.
  • Add a CI workflow and branch protection requirement to run the test suite on pull requests, main, and the merge queue.

Bug Fixes:

  • Replace the placeholder test target with an executable repository validation suite.
  • Correct OpenSSF Best Practices responses to reflect the repository’s testing, continuous integration, test policy, and release model.

Enhancements:

  • Document the test command, dependencies, coverage, and policy in the README.
  • Add pinned, hash-verified test dependencies and regenerate project context for the updated repository standards.

Build:

  • Update the Makefile to support configurable Python execution and run the unittest suite through make test.

CI:

  • Add the Tests workflow with pinned checkout, read-only permissions, draft protection, and merge-queue coverage.
  • Require the make test status check in the main branch ruleset.

Documentation:

  • Document the repository test suite and requirements in README.md.

Tests:

  • Add unit, defect, and boundary coverage for labels, Best Practices metadata, required workflows, test policy, and Markdown link resolution.

Summary by CodeRabbit

  • New Features
    • Added a make test command and automated test runs for pull requests, updates to the main branch, and merge queues.
    • Added checks for published labels, best-practices answers, required status checks, and links in Markdown documentation.
  • Documentation
    • Documented how to run the test suite, its requirements, and the policy for adding or updating tests.
  • Maintenance
    • Updated repository guidance to apply across project types and made successful test runs a required check.

OpenSSF Scorecard reports CII-Best-Practices at in_progress (code
scanning alert 9). The badge cannot reach passing while four MUST
criteria are Unmet: test, test_invocation, test_policy and
tests_are_added. test_continuous_integration (SHOULD) was Unmet too.

- tests/: a Python unittest suite (34 tests) for what the repository
  publishes: labels.yml in the shape EndBug/label-sync reads, the
  .bestpractices.json answers, and the relative links of the rendered
  Markdown files. Each check has a passing case, a planted defect it
  refuses and the boundary between them.
- Makefile: make test runs the suite (PYTHON overrides the
  interpreter); make verify-all runs it with the other checks.
- .github/workflows/test.yml: runs make test on pull requests, on main
  and in the merge queue, with PyYAML pinned by hash in
  .config/tests/requirements.txt.
- README.md: a Tests section with the test policy.
- .bestpractices.json: test, test_invocation, test_policy,
  tests_are_added and test_continuous_integration become Met;
  release_notes becomes N/A (nothing consumes a release; GitHub serves
  the files from main).
- The engine renders the new "make test" job as a required status
  check in .github/rulesets/main.json and refreshed the Paperclip
  harness (.paperclip/, register hash in .standards.yaml).

Refs: https://github.com/golusoris/.github/security/code-scanning/9
Signed-off-by: lusoris <lusoris@proton.me>
The link check matched links with regular expressions, which missed
angle-bracket targets with spaces, titles in parentheses, nested image
links and some code spans and fences. It now parses each file with
markdown-it-py (CommonMark plus tables) and reads the targets from the
link, image and reference-definition tokens, so it sees the links the
renderer sees. Each target is checked once, URL-decoded, and refused
when it resolves outside the repository.

The label check now strips whitespace before it compares names and
refuses blank aliases. The tests run as a package (tests/__init__.py,
discover -t .), and markdown-it-py is pinned with hashes in
.config/tests/requirements.txt.

Signed-off-by: lusoris <lusoris@proton.me>
The engine's workflow trigger check (HISS-18) accepts a draft step only
with the hosted gate's exact title and message. The Tests workflow used
its own wording, so the check reported that the job runs on a draft.
It now carries the same step as the REUSE workflow.

Signed-off-by: lusoris <lusoris@proton.me>
The comment above PYTHON named PyYAML alone. The suite also parses
Markdown links with markdown-it-py since the link checks moved to it,
so the comment now names both, as README.md already does.

Signed-off-by: lusoris <lusoris@proton.me>
The pre-push docstrings gate needs 80 % coverage (.coderabbit.yaml);
the suite had 8 of 61. Each test and helper now names the case it
covers: the input shape and the outcome it expects. Coverage is 61 of
61 and no test body changed.

Signed-off-by: lusoris <lusoris@proton.me>
@sourcery-ai

sourcery-ai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Reviewer's Guide

Implementation

Implement the reusable repository checks for labels, Best Practices answers, required CI checks, the README test policy, and Markdown links.

  • tests/repo_checks.py L1-354

Configuration

Wire the test suite into the Makefile, CI workflow, required-check ruleset, and pinned test dependencies, and refresh generated repository context.

  • .config/tests/requirements.in L1-9
  • .config/tests/requirements.txt L1-85
  • .github/rulesets/main.json L58-60
  • .github/workflows/test.yml L1-50
  • .paperclip/harness.json L14-17
  • .paperclip/rules.md L25-29
  • .standards.yaml L33
  • Makefile L1-8, L28

Update the Best Practices statuses and justifications to reflect the new suite, policy, CI, and release model.

  • .bestpractices.json L42-43, L68-79, L133

Documentation

Document how to run the suite, what it checks, and the policy for adding tests.

  • README.md L13-37

Tests

Add passing, defect, and boundary tests for the published labels, Best Practices answers, and link checks.

  • tests/__init__.py L1
  • tests/test_bestpractices.py L1-203
  • tests/test_labels.py L1-130
  • tests/test_markdown_links.py L1-245

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

This pull request adds tests that validate repository labels, best-practices answers, and Markdown links. It adds local and CI test execution, documents the test suite, updates test-related declarations, and removes language-specific requirements from paperclip rules.

Changes: Repository Test Suite

Layer / File(s) Summary
Repository validation and tests
tests/repo_checks.py, tests/test_bestpractices.py, tests/test_labels.py, tests/test_markdown_links.py, tests/__init__.py
Adds validators and tests for labels, best-practices answers, and Markdown links. Checks cover invalid values, duplicate names or aliases, missing local links, and links that leave the repository.
Test execution and CI
.config/tests/requirements.*, Makefile, .github/workflows/test.yml, .github/rulesets/main.json
Adds pinned test dependencies, makes make test run unittest discovery, adds a workflow for pull requests, pushes to main, and merge-queue events, and requires the workflow check.
Test documentation and declarations
README.md, .bestpractices.json
Documents the test command, requirements, workflow, and test scope. Updates best-practices answers to reflect the tests and CI; coverage measurement remains Unmet.

Paperclip Rule Updates

Layer / File(s) Summary
Generalize language-specific rules
.paperclip/harness.json, .paperclip/rules.md, .standards.yaml
Removes language-specific restrictions from the HISS-01, HISS-02, and HISS-07 descriptions while retaining their general requirements. Updates the registered harness checksum.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Other

Sequence Diagram(s)

sequenceDiagram
  participant GitHubEvents
  participant TestWorkflow
  participant PythonEnvironment
  participant MakeTest
  participant UnittestSuite
  GitHubEvents->>TestWorkflow: Trigger on pull request, main push, or merge queue
  TestWorkflow->>PythonEnvironment: Install hash-verified test requirements
  TestWorkflow->>MakeTest: Run make test
  MakeTest->>UnittestSuite: Discover and run tests
Loading

Possibly related PRs

Merge Risk: Minimal · up to 7e4ed

This change adds a repository test suite and a required CI check for it, and updates the related documentation and declarations. No concrete merge-blocking problem was identified.

Security architecture risk: Low · up to 7e4ed

The new test workflow executes contributed code with read-only repository permissions and without persisted checkout credentials. No material security weakness was established in the inspected changes. Live merge-protection configuration and some source coverage remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The principal new attack surface is contributed code executing in the CI job, with potential effects on that job’s workspace, output, and result. The inspected workflow does not grant repository-write authority or configure deployment credentials.

Trust Boundaries and Controls

  • observed — The validators treat Markdown destinations as data rather than fetching them. Link-target confinement applies after URL decoding and path resolution. This control concerns referenced targets; it is not a general sandbox for arbitrary Python test code.

Resilience and Maintainability Implications

  • observed — Markdown defect tests create a separate temporary directory and register cleanup before writing fixtures. Repository link tests read discovered files. This separates fixture mutations from repository content during normal execution and ordinary test failures; cleanup after abrupt runner termination was not verified.
Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "fix: add a test suite so the Best Practices badge can pass" matches the main change. The PR adds a Python unittest suite for labels, Best Practices answers, and Markdown links, plus a make …
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "fix: add a test suite so the Best Practices badge can pass" matches the main change. The PR adds a Python unittest suite for labels, Best Practices answers, and Markdown links, plus a make test target and a CI workflow. The title names the primary change. Its "fix:" prefix is a conventional-commit label and does not affect the match. The PR objectives say the badge level changes only after a separate save step, so the title states a goal that the diff only partly delivers. That gap does not make the title unrelated, so the check passes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 32 functions across 5 files. (10 skipped: 10 unsupported.)

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 833064f and 7e4ed39.

Files selected for processing: 15

  • .bestpractices.json
  • .config/tests/requirements.in
  • .config/tests/requirements.txt
  • .github/rulesets/main.json
  • .github/workflows/test.yml
  • .paperclip/harness.json
  • .paperclip/rules.md
  • .standards.yaml
  • Makefile
  • README.md
  • tests/__init__.py
  • tests/repo_checks.py
  • tests/test_bestpractices.py
  • tests/test_labels.py
  • tests/test_markdown_links.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@codacy-production

codacy-production Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 238 complexity · 0 duplication

Metric Results
Complexity 238
Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

⚠️ Resolve issues before merging

A validation path can crash on malformed published data instead of reporting a validation failure, which should be fixed before merging.

The suite covers the core label, Best Practices, and Markdown-link scenarios, but two acceptance areas remain unverified: CI configuration enforcement and the related Best Practices status/evidence.

Test suggestions

  • Validate valid labels, colors, aliases, duplicate detection, malformed entries, and boundary color values.
  • Validate Best Practices status values, paired justifications, unknown keys, empty documents, and boundary accepted values.
  • Extract and validate inline links, images, reference links, anchors, external URLs, encoded paths, link titles, and repository escapes.
  • Verify all discovered repository Markdown files have resolvable relative links.
  • Verify the CI workflow and required-check configuration execute and enforce make test.
  • Verify the Best Practices test-continuous-integration answer is Met and cites the CI workflow.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify the CI workflow and required-check configuration execute and enforce make test.
2. Verify the Best Practices test-continuous-integration answer is Met and cites the CI workflow.

Make this review better

TIP Add a ticket key to the branch or title so the reviewer can check the PR against it. Or tell us which ticket tracker you use here.

TIP How was this review? Give us feedback

Comment thread tests/repo_checks.py Resolved
Comment thread tests/test_bestpractices.py Resolved

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 13 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path=".github/workflows/test.yml" line_range="22" />
<code_context>
+
+jobs:
+  test:
+    name: make test
+    runs-on: ubuntu-26.04
+    timeout-minutes: 10
</code_context>
<issue_to_address>
**Other pull requests get blocked**

When the live ruleset requires `make test` before this PR merges and another PR needs to merge, or this PR is abandoned, other pull requests based on `main` do not run this workflow, so GitHub cannot produce the newly required `make test` check for them and blocks their merges. If this PR stops before merging, that check remains required without a workflow on `main`.

Enable the required check only after the workflow is on `main`, or otherwise keep a working path for PRs that cannot yet produce it.
</issue_to_address>

### Comment 2
<location path="tests/test_bestpractices.py" line_range="6" />
<code_context>
+import json
+import unittest
+
+from .repo_checks import ROOT, check_answers
+
+# Criteria a passing badge needs Met (or N/A where the criterion allows it)
</code_context>
<issue_to_address>
**Documented test command fails**

When a contributor runs the `python3 -m unittest discover -s tests` command cited in the test-invocation justification, `unittest` imports the test modules outside the `tests` package, so the relative import in `tests/test_bestpractices.py` raises `ImportError` and the documented command fails.

Update the cited invocation to include `-t .` so `unittest` imports the tests as a package.

Also at `tests/test_bestpractices.py:29`, `Makefile:28`.
</issue_to_address>

### Comment 3
<location path="tests/repo_checks.py" line_range="102" />
<code_context>
+    """Return the link and image targets of a Markdown text, code excluded.
+
+    Each target appears once, in document order, followed by the targets of
+    reference definitions no link uses. Raw HTML is left unchecked.
+    """
+    env = {}
</code_context>
<issue_to_address>
**Broken HTML links pass checks**

When a rendered Markdown file contains a relative link or image destination in raw HTML, `link_targets` only reads Markdown link and image tokens, so raw HTML `href` and `src` values never reach `check_links` and broken destinations pass the suite.

Include relative `href` and `src` destinations from raw HTML in the targets checked by `check_links`.

Also at `tests/repo_checks.py:103`, `tests/repo_checks.py:109-114`.
</issue_to_address>

### Comment 4
<location path="tests/repo_checks.py" line_range="139" />
<code_context>
+    for target in relative_targets(path):
+        local = target.split("#", 1)[0].split("?", 1)[0]
+        unquoted = unquote(local)
+        base = root if unquoted.startswith("/") else path.parent
+        target_path = (base / unquoted.lstrip("/")).resolve()
+        if not target_path.is_relative_to(root.resolve()):
</code_context>
<issue_to_address>
**Root links point elsewhere**

When a rendered Markdown link uses a leading-slash destination that matches a repository file, `check_links` resolves the destination under the repository root, but GitHub renders it from the site origin, so the check passes while readers are sent elsewhere.

Resolve leading-slash destinations according to the rendered GitHub link behavior instead of treating them as repository-root paths.

Also at `tests/test_markdown_links.py:77`.
</issue_to_address>

### Comment 5
<location path="tests/repo_checks.py" line_range="89" />
<code_context>
+    justified = {key[: -len("_justification")] for key in data if key.endswith("_justification")}
+    for criterion in sorted(statuses):
+        value = data[criterion + "_status"]
+        if value not in STATUS_VALUES:
+            errors.append(f"{criterion}_status: {value!r} is not one of {sorted(STATUS_VALUES)}")
+        text = data.get(criterion + "_justification")
</code_context>
<issue_to_address>
**Invalid statuses crash validation**

When a `.bestpractices.json` status is an array or object, `check_answers` tests membership in `STATUS_VALUES` before checking the value's type, so the unhashable value raises `TypeError` instead of returning a validation error.

Check that each status has an appropriate scalar type before testing whether it belongs to `STATUS_VALUES`.
</issue_to_address>

### Comment 6
<location path="tests/test_bestpractices.py" line_range="10" />
<code_context>
+
+# Criteria a passing badge needs Met (or N/A where the criterion allows it)
+# that this repository answers with its own test suite and policy.
+TEST_CRITERIA = ("test", "test_invocation", "test_policy", "tests_are_added")
+
+
</code_context>
<issue_to_address>
**Required answers escape checks**

When a currently improved criterion omitted from `TEST_CRITERIA` is changed to `Unmet` or removed, `check_answers` accepts valid statuses or absent criterion pairs, so the suite passes even though the required CI or release-notes answer no longer supports the passing badge.

Include every required criterion, including `test_continuous_integration` and `release_notes`, in the criteria asserted as Met.
</issue_to_address>

### Comment 7
<location path="tests/repo_checks.py" line_range="68-70" />
<code_context>
+        return ["labels.yml must be a non-empty list"]
+    errors = []
+    seen = {}
+    for index, entry in enumerate(data):
+        errors.extend(check_label(entry, index))
+        for name in label_names(entry):
+            if not isinstance(name, str):
+                continue
</code_context>
<issue_to_address>
**Large inputs can stall tests**

When a published YAML/JSON document, Markdown file, or discovered Markdown-file set grows large enough, `check_labels`, `check_answers`, `link_targets`, `relative_targets`, `check_links` and `markdown_files` iterate input collections without a scalar cap. Large repository files or documents can make `make test` exceed the workflow timeout and fail the required check.

Set scalar maximums for these collections and reject inputs that exceed them before iteration.

Also at `tests/repo_checks.py:84-87`, `tests/repo_checks.py:94`, `tests/repo_checks.py:109-110`, `tests/repo_checks.py:116-117`, `tests/repo_checks.py:123`, `tests/repo_checks.py:136`, `tests/repo_checks.py:150`, `tests/test_markdown_links.py:27`.
</issue_to_address>

### Comment 8
<location path="tests/repo_checks.py" line_range="41-47" />
<code_context>
+        return [f"entry {index}: not a mapping"]
+    errors = [f"entry {index}: unknown key {key!r}" for key in sorted(set(entry) - LABEL_KEYS)]
+    name = entry.get("name")
+    if not isinstance(name, str) or not name.strip():
+        errors.append(f"entry {index}: name must be a non-empty string")
+    color = entry.get("color")
+    if not isinstance(color, str) or not LABEL_COLOR.fullmatch(color):
+        errors.append(f"entry {index}: color must be six hexadecimal digits")
+    if "description" in entry and not isinstance(entry["description"], str):
+        errors.append(f"entry {index}: description must be a string")
+    aliases = entry.get("aliases", [])
+    if not isinstance(aliases, list) or not all(isinstance(a, str) and a.strip() for a in aliases):
</code_context>
<issue_to_address>
**Overlong labels pass validation**

When a label name exceeds GitHub's 50-character limit or a description exceeds its 100-character limit, `check_label` checks only that names are non-empty and descriptions are strings, so an over-limit value passes `make test`; the `sync-labels` workflow then submits it to GitHub, which rejects the label and prevents synchronization.

Enforce GitHub's name and description length limits in `check_label` and add boundary tests for each.
</issue_to_address>

### Comment 9
<location path="tests/repo_checks.py" line_range="31" />
<code_context>
+
+# Markdown files the organisation renders: the profile, the community health
+# files and the decision records.
+MARKDOWN_GLOBS = ("*.md", "profile/*.md", "docs/**/*.md")
+EXTERNAL = re.compile(r"[a-zA-Z][a-zA-Z0-9+.-]*:")
+
</code_context>
<issue_to_address>
**Nested profile files escape link checks**

When a profile Markdown file is added below a subdirectory of `profile/`, `profile/*.md` discovers only Markdown files directly under `profile/`, so a Markdown file in a profile subdirectory is omitted from `markdown_files` and its broken relative links do not fail the repository test.

Make profile Markdown discovery recursive and add a fixture proving a nested file with a broken link is checked.
</issue_to_address>

### Comment 10
<location path="tests/repo_checks.py" line_range="143" />
<code_context>
+        target_path = (base / unquoted.lstrip("/")).resolve()
+        if not target_path.is_relative_to(root.resolve()):
+            errors.append(f"{path.relative_to(root)}: {target} leaves the repository")
+        elif not target_path.exists():
+            errors.append(f"{path.relative_to(root)}: {target} names no file")
+    return errors
</code_context>
<issue_to_address>
**Broken image targets pass as directories**

When a Markdown image target resolves to an existing directory, `check_links` accepts any existing path, including a directory, so a relative image target that resolves to a directory produces no error even though the image cannot load and the repository test passes.

Retain whether each target is an image and require image targets to resolve to files; add a directory-target defect test.
</issue_to_address>

### Comment 11
<location path="tests/test_bestpractices.py" line_range="34" />
<code_context>
+    def test_badge_names_this_project(self):
+        """README.md contains the OpenSSF badge URL for project 16160."""
+        readme = (ROOT / "README.md").read_text(encoding="utf-8")
+        self.assertIn("https://www.bestpractices.dev/projects/16160/badge", readme)
+
+
</code_context>
<issue_to_address>
**Removed policy still passes**

When the README test-policy section is removed or its requirements are weakened, `test_badge_names_this_project` checks only the badge URL, so removing the README test policy leaves the suite green while `.bestpractices.json` still claims `test_policy` is Met. The stated policy can disappear without the tests detecting that the badge evidence is no longer true.

Add a test that verifies the README contains the required test-policy commitments.
</issue_to_address>

### Comment 12
<location path="tests/test_markdown_links.py" line_range="106" />
<code_context>
+
+    def test_target_resolution_and_escape(self):
+        """Query, root-anchor and // targets pass; a path out of the repo is refused."""
+        (self.root.parent / "outside.md").write_text("# Outside\n", encoding="utf-8")
+        self.assertEqual(self.check("[x](?query) [y](/#anchor)\n"), [])
+        self.assertEqual(self.check("[p](//example.org/x)\n"), [])
</code_context>
<issue_to_address>
**Test leaves shared file behind**

When the shared temporary-directory parent already contains an `outside.md` file, `test_target_resolution_and_escape` overwrites that file, and cleanup removes only the temporary test root, leaving the replacement behind.

Create the escape target in an isolated fixture and avoid overwriting files outside the test's temporary root.
</issue_to_address>

### Comment 13
<location path=".github/workflows/test.yml" line_range="8-12" />
<code_context>
+name: Tests
+
+'on':
+  pull_request:
+    types: [opened, synchronize, reopened, ready_for_review]
+  push:
+    branches: ['main']
+  merge_group:
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.number || github.run_id }}
</code_context>
<issue_to_address>
**Tests run without diff filtering**

The workflow starts `make test` for every pull request, main push and merge-group event without using `praetorctl ci filter`, so unrelated changes also run the gate instead of being diff-filtered.

Use `praetorctl ci filter` to determine when this workflow should run based on the changed files.

Also at `.github/workflows/test.yml:44`.
</issue_to_address>

Sourcery assessment

Approval pending. 12 findings to address first.

Blocking findings: .github/workflows/test.yml:22, tests/test_bestpractices.py:6, tests/repo_checks.py:102, tests/repo_checks.py:139, tests/repo_checks.py:89, and 7 more


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread .github/workflows/test.yml Resolved
Comment thread tests/test_bestpractices.py Resolved
Comment thread tests/repo_checks.py Resolved
Comment thread tests/repo_checks.py Resolved
Comment thread tests/repo_checks.py Resolved
Comment thread tests/repo_checks.py Resolved
Comment thread tests/repo_checks.py Resolved
Comment thread tests/test_bestpractices.py Resolved
Comment thread tests/test_markdown_links.py Resolved
Comment thread .github/workflows/test.yml Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments: 1

.bestpractices.json: 1

79-79: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Tie the tests_are_added claim to a verifiable commit.

The claim refers to "the most recent change". This text becomes stale after the next change to a published file. The commits link points to a moving target. Cite the PR number or commit hash instead.

Review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 833064f and 4705c21.

Files selected for processing: 15

  • .bestpractices.json
  • .config/tests/requirements.in
  • .config/tests/requirements.txt
  • .github/rulesets/main.json
  • .github/workflows/test.yml
  • .paperclip/harness.json
  • .paperclip/rules.md
  • .standards.yaml
  • Makefile
  • README.md
  • tests/__init__.py
  • tests/repo_checks.py
  • tests/test_bestpractices.py
  • tests/test_labels.py
  • tests/test_markdown_links.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

The review of pull request 12 found checks that crashed, missed cases
or claimed more than they tested. Each fix comes with a test that fails
without it (26 planted defects, all refused).

- A status given as a list or mapping is refused, not a TypeError.
- Label names, aliases and descriptions are held to the GitHub limits
  of 50 and 100 characters.
- Every collection the checks walk has a scalar cap (HISS-02).
- Nested profile Markdown files are found, an image that names a
  directory is refused, and raw HTML a, img and source tags are read.
- A required status check in .github/rulesets/main.json must name a
  job of a workflow that runs on every pull request, with no path
  filter that could skip it.
- The README test policy and the make test command are checked
  against the Best Practices answers that cite them.
- The link tests keep their temporary files inside their own
  directory.
- Two answer texts now quote the real test command and name the pull
  request that added the suite.

Signed-off-by: lusoris <lusoris@proton.me>
@lusoris

lusoris commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

All review items are answered in 7e4ed39: 12 are fixed with tests, and 3 have replies citing GitHub Docs for why no change is needed. Two review notes have no thread of their own, so they are answered here.

  • CodeRabbit, .bestpractices.json:79 (tests_are_added). Fixed. The justification now names pull request 12 and links it, instead of "the most recent change" and the moving commits view. It also states that the README test policy requires the same of every later change.
  • Codacy, the two open test suggestions.
    • Required-check enforcement: test_required_status_checks_match_workflows fails when a required context in .github/rulesets/main.json names no job of a workflow that runs on every pull request. make test from .github/workflows/test.yml is that job today.
    • CI criterion: test_ci_and_release_notes_criteria asserts that test_continuous_integration is Met and cites .github/workflows/test.yml.

Every fix was checked by planting its defect back in a scratch copy: all 26 planted defects turn the suite red, and the unchanged copy stays green.

@lusoris
lusoris merged commit 649f0fd into main Nov 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant