Repository navigation
Add reusable, verified repository statistics infographic skill - #67015
Conversation
Extract a portable six-panel renderer with immutable typed data, exact count arithmetic, independent cached API and pinned Git audits, and regression/property tests. Document evidence requirements and attribution limitations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No Go or JavaScript test files were added or modified in this PR (only a Python test file, .github/skills/statistics-infographic/tests/test_statistics.py, which is detected but not scored per the Test Quality Sentinel scope). All pre-fetched Go/JS test signal files (go-new-test-funcs.txt, go-modified-test-funcs.txt, js-new-test-funcs.txt, js-changed-test-files.txt, go-testmain-funcs.txt) are empty. Test Quality Sentinel skipped - no reviewable signal in scope.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Testing safeoutputs reachability from this runner.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
L82-91: delete: optional precomputed display-value validation. Derive every shown percentage and ratio from the canonical counts only; the renderer already does, so this extra float/tolerance schema is unused flexibility.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #67015 · codex · gpt56 · 17.7 AIC · ⌖ 6.56 AIC · ⊞ 13.4K
Comment /ponytail to run again
| return "n/a" if total == 0 else decimal(Fraction(100 * part, total), places) + "%" | ||
|
|
||
|
|
||
| def check_published(record: dict[str, object], key: str, expected: str) -> None: |
There was a problem hiding this comment.
L82-91: delete: optional precomputed display-value validation. Derive every shown percentage and ratio from the canonical counts only; the renderer already does, so this extra float/tolerance schema is unused flexibility.
There was a problem hiding this comment.
REQUEST_CHANGES
Two blocking correctness bugs slipped into the new statistics-infographic tooling: the Git auditor never recurses into nested paths, and the renderer still crashes on the zero-line dataset that it claims to support.
Blocking themes
- Git audit correctness:
audit_git()inventories only top-level tree entries, so ordinary repositories with code under directories fail reconciliation for the wrong reason. - Renderer edge case: the code-map panel divides by
s.linesbefore the empty-state branch, so a valid all-zero Go or JS input raises instead of rendering the documented fallback.
These are both user-visible failures in the new reusable skill, so I am requesting changes before merge.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 50.7 AIC · ⌖ 5.43 AIC · ⊞ 19.8K
Comment /review to run again
| def audit_git(s: Statistics, repo: Path) -> None: | ||
| require(git(repo, ["rev-parse", "--is-shallow-repository"]).strip() == b"false", "Git audit requires complete, non-shallow history") | ||
| blobs: list[tuple[str, str]] = [] | ||
| for entry in git(repo, ["ls-tree", "-rz", s.commit]).split(b"\0"): |
There was a problem hiding this comment.
This Git audit only walks the top-level tree, so any repository with Go or JS files under subdirectories will be counted as having zero blobs and will fail the code-map reconciliation.
💡 Recurse through the commit tree before batching blob reads.
git ls-tree -z (commit) does not descend into subdirectories, so blobs becomes incomplete or empty for normal repositories. Use git(repo, ["ls-tree", "-r", "-z", s.commit]) here so the pinned blob inventory matches the documented Git audit behavior.| language_lines = sum(g.lines for g in groups) | ||
| if language_lines == 0: | ||
| continue | ||
| block_width = w * language_lines / s.lines |
There was a problem hiding this comment.
The empty-state guard is too late: this panel still divides by s.lines before reaching the zero-data branch, so a valid zero-line dataset crashes with ZeroDivisionError instead of rendering the fallback text.
💡 Move the zero-line branch above the treemap math.
Statistics.from_sources() allows all four groups to have zero lines, and this panel already has a No nonblank Go / JS lines fallback. But block_width = w * language_lines / s.lines runs first, so the renderer never reaches that UI. Wrap the percentage and treemap logic in an if s.lines == 0 branch or return early for the empty case.There was a problem hiding this comment.
🟡 Changes recommended
Temporal evidence validation, replacement-object isolation, and numeric overflow handling need correction.
3 open findings
What changed in this PR
Adds a reusable skill for generating and independently auditing repository statistics infographics.
Changes:
- Adds validated statistics models and deterministic six-panel PNG rendering.
- Adds API/Git evidence auditing and a documented data contract.
- Adds synthetic fixtures, dependencies, and comprehensive tests.
| File | Description |
|---|---|
SKILL.md |
Documents generation, verification, and maintenance. |
scripts/statistics_data.py |
Validates and models statistics. |
scripts/render_infographic.py |
Renders and atomically saves the infographic. |
scripts/audit_sources.py |
Reconciles API and Git evidence. |
tests/test_statistics.py |
Tests arithmetic, rendering, auditing, and failures. |
references/data-contract.md |
Defines evidence schemas and interpretations. |
examples/example.json |
Provides synthetic input data. |
requirements.txt |
Declares runtime dependency. |
requirements-dev.txt |
Declares test and type-check dependencies. |
🧠 Review effort: Balanced
|
|
||
|
|
||
| def git(repo: Path, args: list[str], stdin: bytes | None = None) -> bytes: | ||
| env = {**os.environ, "GIT_NO_LAZY_FETCH": "1", "GIT_TERMINAL_PROMPT": "0"} |
| if state == "MERGED": | ||
| delta = timestamp(p["mergedAt"]) - created | ||
| micros = (delta.days * 86400 + delta.seconds) * 1000000 + delta.microseconds | ||
| require(micros >= 0, "merge precedes PR creation") | ||
| elapsed.append(micros) |
| def number(value: object, name: str) -> float: | ||
| if not isinstance(value, (int, float)) or isinstance(value, bool): | ||
| raise DataError(f"{name}: expected a finite number") | ||
| result = float(value) | ||
| require(math.isfinite(result) and result >= 0, f"{name}: expected a finite nonnegative number") | ||
| return result |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs (pr-triage recommended /tdd and /grill-with-docs; substituted /codebase-design for the lru_cache lifetime finding since test coverage here is already very strong). No blocking issues — commenting only.
📋 Key Themes & Highlights
Key Themes
- Instance-method
lru_cachelifetime risk inrender_infographic.py'sCanvas.face— fine for one-shot CLI use, but leaksCanvasinstances if this module is ever reused in a long-lived process or batch loop. - Doc/code contract gap on the
assumptionfield instatistics_data.py—data-contract.mdpromises the no-issue-human assumption "must be explicitly authorized," but validation only checks for a nonempty string, so the authorization guarantee isn't actually enforced in code.
Positive Highlights
- ✅ Excellent test discipline: Decimal-oracle property tests for rounding/quantiles, source-mutation regression tests, deterministic-pixel rendering checks, and CLI failure-preserves-output tests all cover edge cases thoroughly.
- ✅
audit_sources.py's independent reconciliation against raw JSONL and pinned Git objects (no fetch, no credentials) is a strong, well-isolated verification layer that matches the stated "verify accuracy" design goal. - ✅ Frozen dataclasses + exact
Fraction-based arithmetic avoid float-precision bugs in displayed percentages/ratios — a good deep-module choice for a rendering pipeline that must not silently corrupt counts. - ✅
render_infographic.py'sLayoutError+ atomic save (temp file +Image.verify()+replace) correctly preserves existing output on failure, matching the SKILL.md's documented behavior.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 93.1 AIC · ⌖ 14.6 AIC · ⊞ 10.1K
Comment /matt to run again
| self.labels: list[tuple[Bbox, str]] = [] | ||
| self.clip = (0, 0, WIDTH, HEIGHT) | ||
|
|
||
| @lru_cache(maxsize=32) |
There was a problem hiding this comment.
[/codebase-design] @lru_cache on a bound instance method (Canvas.face) caches by (self, size, bold), so every Canvas instance the decorator ever sees is kept alive for the lifetime of the class attribute's cache — a well-known Python footgun. It's harmless for a single one-shot render, but the module becomes unsafe to reuse in a batch/loop context (e.g. rendering many repos in one process) without memory growth, which cuts against the "portable, reusable skill" framing of this PR.
💡 Suggested fix
class Canvas:
def __init__(self, fonts: tuple[Path, Path]) -> None:
self.fonts = fonts
self._faces: dict[tuple[int, bool], ImageFont.FreeTypeFont] = {}
...
def face(self, size: int, bold: bool = False) -> ImageFont.FreeTypeFont:
key = (size, bold)
if key not in self._faces:
self._faces[key] = ImageFont.truetype(str(self.fonts[int(bold)]), size)
return self._faces[key]This keeps per-size font caching but scopes it to the instance, so a Canvas is fully garbage-collectible once it goes out of scope.
@copilot please address this.
| require(components["direct_aw_prs"] + components["cca_aw_issue_prs"] == outcomes[0].total, "AW components disagree") | ||
| require(components["cca_human_issue_prs"] + components["cca_no_issue_assumed_human_prs"] == outcomes[1].total, "human components disagree") | ||
| require(sum(components[key] for key in component_keys[4:]) == outcomes[2].total, "other components disagree") | ||
| assumption = string(o.get("assumption"), "assumption") |
There was a problem hiding this comment.
[/grill-with-docs] references/data-contract.md states the no-issue-human assumption "must be explicitly authorized" and recorded in assumption, but the implementation only checks string(o.get("assumption"), "assumption"), i.e. any nonempty string passes. There's no enforced vocabulary or flag tying this field to actual authorization — the doc promise is aspirational, not verified by code, so a careless caller could satisfy the contract with an unrelated non-empty string.
💡 Suggestion
Either relax the doc wording to say the field is informational/disclosed-but-unverified, or strengthen validation, e.g. requiring the assumption text to be non-boilerplate (reject a short default placeholder) or requiring an explicit separate boolean authorized: true field when cca_no_issue_assumed_human_prs > 0. As written, the gap between the documented contract and the enforced contract could mislead a future maintainer who assumes this field is a real control.
@copilot please address this.
There was a problem hiding this comment.
Reviewed with the Impeccable harden/audit lenses (new feature: reusable rendering + audit skill).
Found one verified, reproducible correctness bug in the font-discovery fallback (render_infographic.py): the Linux Liberation Sans candidate path (/usr/share/fonts/truetype/liberation2/...) matches Fedora/RHEL packaging, not Debian/Ubuntu (/usr/share/fonts/truetype/liberation/..., confirmed present on this runner). Since candidates are tried in order and any missing file in a pair is silently skipped, Debian/Ubuntu (the common CI base) always falls through to DejaVu Sans, contradicting the SKILL.md claim of Liberation Sans support there. Left an inline comment with the fix.
Rest of the arithmetic/validation/audit logic (exact-rational percentages, half-up rounding, histogram contiguity, Git immutability checks) looks sound on read-through; no other blocking issues found in the diff.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 113.1 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
Comments that could not be inline-anchored
.github/skills/statistics-infographic/scripts/render_infographic.py:702
On Debian/Ubuntu (the common CI/Linux base, including this runner), Liberation Sans ships at /usr/share/fonts/truetype/liberation/LiberationSans-*.ttf, not liberation2/ (that path matches Fedora/RHEL packaging). Verified locally: /usr/share/fonts/truetype/liberation2/ does not exist while /usr/share/fonts/truetype/liberation/LiberationSans-Regular.ttf does.
Because discover_fonts() tries candidates in order and silently falls through on a missing pair, this candidate never matches on…
|
🎉 This pull request is included in a new release. Release: |
Why
Turn the session-specific statistics infographic into a reusable skill so repository reports can be regenerated from saved evidence instead of hard-coded numbers, dates, fonts, or session paths.
Approach
The renderer rejects unreadable layouts rather than silently truncating data and preserves existing output on failure. Attribution remains an inference, not proof of session initiation; the no-issue human assumption requires explicit authorization. Code scope is Go/JavaScript, and test-line share is not test coverage. Real datasets and generated images are not committed.
Validation
python -m unittest discover -s .github/skills/statistics-infographic/tests -v: 17 tests passed, including independent Decimal-oracle property tests.python -m mypy --strict .github/skills/statistics-infographic/scripts: all three production modules passed.make agent-report-progress: passed; no Go changes required impacted Go tests or workflow recompilation.