Skip to content

Commit 8563cd7

Browse files
datalogics-cgreendatalogics-cgreen
authored
Merge pull request #23 from datalogics-kam/pdfcloud-5575-add-pr-auditor-skill
PDFCLOUD-5575 Add PR Review Auditor skill
2 parents 20bce70 + ae0f80e commit 8563cd7

7 files changed

Lines changed: 337 additions & 5 deletions

File tree

‎.agents/skills/pr-review-auditor/SKILL.md‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
---
2+
name: pr-review-auditor
3+
description: Review the currently checked-out PR by discovering its upstream base branch with GitHub CLI, diffing base..HEAD, and performing a correctness-first code review that enforces repository policy with special priority on *GUIDELINES*.md rules and then AGENTS.md. Use when asked to do a full PR review, /review-style audit, commit-range quality check, or policy-compliance review of branch changes.
4+
---
5+
6+
# PR Review Auditor
7+
8+
## Overview
9+
10+
Review branch changes from PR base to `HEAD` with findings-first output.
11+
Prioritize correctness risks, then CI regressions, then test gaps, and enforce
12+
policy precedence: `*GUIDELINES*.md` before `AGENTS.md`.
13+
14+
## Workflow
15+
16+
1. Collect context from the active branch. Run
17+
`.agents/skills/pr-review-auditor/scripts/collect_pr_context.sh` from repo
18+
root to detect:
19+
20+
- checked-out branch,
21+
- PR number and base branch (via `gh pr view`),
22+
- merge-base commit,
23+
- review range (`<base_remote>/<base>..HEAD`),
24+
- changed files,
25+
- available policy files (`*GUIDELINES*.md`, `AGENTS.md`).
26+
27+
2. Inspect commits and patches in range. Run
28+
`.agents/skills/pr-review-auditor/scripts/review_range.sh` with the range
29+
from step 1. This prints:
30+
31+
- commits with full messages (`git log --format=fuller`),
32+
- per-commit changed files,
33+
- unified patches for detailed behavior review.
34+
35+
3. Run a three-pass policy protocol (Markdown-only). Pass 1: Rule extraction
36+
37+
- Read all `*GUIDELINES*.md` files first, then `AGENTS.md`.
38+
- Extract rules section-by-section by heading. For each heading, list actionable
39+
rules before moving to the next heading.
40+
- Extract every actionable rule into a numbered list with source citation.
41+
- Keep wording close to source text and avoid merging unrelated rules.
42+
- Minimum extraction floors:
43+
- `TESTING_GUIDELINES.md`: at least 20 rules unless a heading-by-heading audit
44+
proves fewer.
45+
- `AGENTS.md`: at least 15 rules unless a heading-by-heading audit proves
46+
fewer.
47+
- If below floor, include an explicit justification section that names each
48+
heading and why no additional actionable rule exists. Pass 2: Evidence
49+
mapping
50+
- For each extracted rule, assign `pass`, `fail`, or `n/a`.
51+
- Provide concrete evidence (`file:line`, command output, or explicit absence).
52+
- If rules conflict, cite both and follow `*GUIDELINES*.md`. Pass 3: Adversarial
53+
gap check
54+
- Re-read diffs and policy files only to find missed rules/findings.
55+
- Add new findings only when they include new source citations and evidence.
56+
- Require at least 5 "candidate missed rules" per policy file during this pass,
57+
then mark each as `new rule` or `duplicate` with rationale.
58+
59+
4. Perform review in severity order.
60+
61+
- Correctness/behavioral regressions.
62+
- CI and workflow regressions.
63+
- Test gaps or weak assertions.
64+
- Secondary maintainability/style concerns.
65+
66+
5. Produce findings-first output. List issues with file/line references and
67+
policy citation when applicable. Separate policy findings into two explicit
68+
sections:
69+
70+
- `GUIDELINES findings`
71+
- `AGENTS findings`
72+
73+
6. Provide a remediation plan. Include:
74+
75+
- ordered fix plan,
76+
- tests to run and why,
77+
- tests not run and why,
78+
- assumptions and open questions.
79+
80+
## Output Requirements
81+
82+
- Findings first, ordered by severity.
83+
- Include commit-message quality notes when relevant.
84+
- Cite exact paths and lines for each finding.
85+
- Include `files reviewed`, `tests run`, and `tests not run`.
86+
- If no findings exist, state that explicitly and list residual risks.
87+
- Include a `Rule Coverage Matrix` section in Markdown.
88+
- Do not mark the review complete until every extracted rule has a status and
89+
evidence.
90+
- Do not emit internal script-path fallback chatter; run the canonical
91+
`.agents/skills/pr-review-auditor/scripts/*` paths directly.
92+
- Include a `Rule Extraction Summary` with:
93+
- total rules per source file,
94+
- per-heading rule counts,
95+
- whether extraction floors were met,
96+
- explicit justification if any floor was not met.
97+
98+
## Rule Coverage Matrix Template
99+
100+
Use this exact table shape:
101+
102+
| Rule ID | Source | Rule Summary | Status (pass/fail/n-a) | Evidence |
103+
| ------- | --------------------- | ------------ | ---------------------- | ------------------ |
104+
| G-001 | TESTING_GUIDELINES.md | ... | pass | tests/test_x.py:42 |
105+
| A-001 | AGENTS.md | ... | fail | src/module.py:87 |
106+
107+
## Trigger Examples
108+
109+
- "Figure out this PR's base branch and do a /review from base to HEAD."
110+
- "Audit all commits in this branch and check AGENTS + GUIDELINES compliance."
111+
- "Review patches in this PR and give me a fix plan with test gaps."
112+
113+
## Resources
114+
115+
- `.agents/skills/pr-review-auditor/scripts/collect_pr_context.sh`: derive PR
116+
base/range and policy file inventory.
117+
- `.agents/skills/pr-review-auditor/scripts/review_range.sh`: print full commit
118+
messages and patches for a range.
119+
- `references/review-checklist.md`: compact review checklist and severity
120+
rubric.

‎.agents/skills/pr-review-auditor/agents/openai.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
interface:
2+
display_name: "PR Review Auditor"
3+
short_description: "Review base..HEAD with GUIDELINES-first policy checks."
4+
default_prompt: "Use $pr-review-auditor to review the current PR from upstream base to HEAD with findings-first output and strict GUIDELINES/AGENTS compliance checks."

‎.agents/skills/pr-review-auditor/references/review-checklist.md‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# PR Review Checklist
2+
3+
## Policy Precedence
4+
5+
1. `*GUIDELINES*.md` rules are highest-priority compliance checks.
6+
2. `AGENTS.md` rules are required secondary checks.
7+
3. If rules conflict, cite both files and follow `*GUIDELINES*.md`.
8+
9+
## Review Scope
10+
11+
- Determine PR base branch and audit `<base_remote>/<base>..HEAD`.
12+
- Review full commit messages, changed files, and relevant patches.
13+
- Focus on behavior, CI impact, tests, then style.
14+
15+
## Three-Pass Completeness Protocol
16+
17+
1. Rule extraction pass:
18+
19+
- Enumerate every actionable rule from `*GUIDELINES*.md`, then `AGENTS.md`.
20+
- Extract heading-by-heading and record per-heading counts.
21+
- Assign stable rule IDs (for example `G-001`, `A-001`).
22+
- Meet extraction floors unless explicitly justified:
23+
- `TESTING_GUIDELINES.md`: >= 20 rules.
24+
- `AGENTS.md`: >= 15 rules.
25+
- If below floor, justify heading-by-heading why no additional actionable
26+
rules exist.
27+
28+
2. Evidence mapping pass:
29+
30+
- Map every rule to `pass`, `fail`, or `n/a`.
31+
- Attach evidence for every status (path/line or concrete command output).
32+
33+
3. Gap pass:
34+
35+
- Re-check changed files and policy text for missing rules/findings.
36+
- Add only findings backed by new source citations and evidence.
37+
- Generate at least 5 missed-rule candidates per policy file, then classify each
38+
as `new rule` or `duplicate` with rationale.
39+
40+
## Severity Order
41+
42+
1. Correctness risk: behavior bug, data loss, runtime failure, API mismatch.
43+
2. CI/workflow regression: broken matrix, wrong triggers, missing job parity.
44+
3. Test gap: missing or weak coverage for changed behavior.
45+
4. Maintainability/style: readability or consistency issues with low break risk.
46+
47+
## Mandatory Output Sections
48+
49+
- Findings (ordered by severity, each with path/line evidence).
50+
- Rule Coverage Matrix (every extracted rule must be represented).
51+
- Rule Extraction Summary (totals per file, per-heading counts, floors met/not
52+
met).
53+
- GUIDELINES findings.
54+
- AGENTS findings.
55+
- Open questions and assumptions.
56+
- Remediation plan with validation steps.
57+
- Files reviewed.
58+
- Tests run.
59+
- Tests not run and why.
60+
61+
## Completion Gate
62+
63+
- The review is incomplete if any extracted rule is missing from the matrix.
64+
- The review is incomplete if any matrix row lacks status or evidence.
65+
- The review is incomplete if extraction floors are missed without explicit
66+
heading-by-heading justification.

‎.agents/skills/pr-review-auditor/scripts/collect_pr_context.sh‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
#!/usr/bin/env bash
2+
set -euo pipefail
3+
4+
branch="$(git rev-parse --abbrev-ref HEAD)"
5+
base_remote="origin"
6+
if git remote get-url upstream > /dev/null 2>&1; then
7+
base_remote="upstream"
8+
fi
9+
10+
context_source="gh-pr-view"
11+
pr_number="N/A"
12+
head_ref="${branch}"
13+
base_ref=""
14+
pr_json=""
15+
gh_error=""
16+
17+
if command -v gh > /dev/null 2>&1 && command -v jq > /dev/null 2>&1; then
18+
gh_error_file="$(mktemp)"
19+
if ! pr_json="$(gh pr view --json number,baseRefName,headRefName 2> "${gh_error_file}")"; then
20+
pr_json=""
21+
fi
22+
if [[ -z "${pr_json}" ]]; then
23+
gh_error="$(< "${gh_error_file}")"
24+
fi
25+
rm -f "${gh_error_file}"
26+
fi
27+
28+
if [[ -n "${pr_json}" && "${pr_json}" != "null" ]]; then
29+
pr_number="$(jq -r '.number' <<< "${pr_json}")"
30+
base_ref="$(jq -r '.baseRefName' <<< "${pr_json}")"
31+
head_ref="$(jq -r '.headRefName' <<< "${pr_json}")"
32+
else
33+
context_source="remote-head-fallback"
34+
remote_head_ref="$(git symbolic-ref --quiet --short "refs/remotes/${base_remote}/HEAD" || true)"
35+
if [[ -z "${remote_head_ref}" ]]; then
36+
echo "error: unable to derive base branch from refs/remotes/${base_remote}/HEAD" >&2
37+
echo "hint: run 'git remote set-head ${base_remote} --auto' and retry" >&2
38+
if [[ -n "${gh_error}" ]]; then
39+
echo "gh pr view: ${gh_error}" >&2
40+
fi
41+
exit 1
42+
fi
43+
base_ref="${remote_head_ref#"${base_remote}"/}"
44+
if [[ -n "${gh_error}" ]]; then
45+
echo "warning: gh pr view failed; using ${base_remote}/HEAD fallback" >&2
46+
echo "gh pr view: ${gh_error}" >&2
47+
fi
48+
fi
49+
50+
fetch_error_file="$(mktemp)"
51+
if ! git fetch --quiet "${base_remote}" "${base_ref}" 2> "${fetch_error_file}"; then
52+
if git rev-parse --verify --quiet "${base_remote}/${base_ref}" > /dev/null; then
53+
echo "warning: git fetch failed; using cached ${base_remote}/${base_ref}" >&2
54+
cat "${fetch_error_file}" >&2
55+
else
56+
echo "error: unable to fetch ${base_remote}/${base_ref} and no local cached ref exists" >&2
57+
cat "${fetch_error_file}" >&2
58+
rm -f "${fetch_error_file}"
59+
exit 1
60+
fi
61+
fi
62+
rm -f "${fetch_error_file}"
63+
64+
merge_base="$(git merge-base "${base_remote}/${base_ref}" HEAD)"
65+
review_range="${base_remote}/${base_ref}..HEAD"
66+
67+
echo "CONTEXT_SOURCE=${context_source}"
68+
echo "PR_NUMBER=${pr_number}"
69+
echo "CURRENT_BRANCH=${branch}"
70+
echo "PR_HEAD_BRANCH=${head_ref}"
71+
echo "PR_BASE_BRANCH=${base_ref}"
72+
echo "PR_BASE_REMOTE=${base_remote}"
73+
echo "MERGE_BASE=${merge_base}"
74+
echo "REVIEW_RANGE=${review_range}"
75+
echo
76+
77+
echo "CHANGED_FILES:"
78+
git diff --name-only "${review_range}"
79+
echo
80+
81+
echo "POLICY_FILES:"
82+
find . -type f \( -name '*GUIDELINES*.md' -o -name 'AGENTS.md' \) | sort

‎.agents/skills/pr-review-auditor/scripts/review_range.sh‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
#!/usr/bin/env bash
2+
set -euo pipefail
3+
4+
range="${1:-}"
5+
if [[ -z "${range}" ]]; then
6+
echo "usage: $(basename "$0") <git-range>" >&2
7+
echo "example: $(basename "$0") upstream/main..HEAD" >&2
8+
exit 1
9+
fi
10+
11+
echo "=== RANGE ==="
12+
echo "${range}"
13+
echo
14+
15+
echo "=== COMMITS (full messages) ==="
16+
git log --format=fuller "${range}"
17+
echo
18+
19+
echo "=== COMMIT FILE SUMMARIES ==="
20+
git log --name-status --format='commit %H%nAuthor: %an <%ae>%nDate: %ad%n%n%s%n%b' "${range}"
21+
echo
22+
23+
echo "=== PATCHES ==="
24+
git log --patch --stat "${range}"

‎AGENTS.md‎

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,38 @@
2323
- `uvx nox -s tests` — create matrix virtualenvs via nox and execute the pytest
2424
session.
2525
- `nox` executes pytest sessions with built-in parallelism; when invoking pytest
26-
directly use `pytest -n 8 --maxschedchunk 2` to mirror the parallel test
26+
directly use `pytest -n auto --maxschedchunk 2` to mirror the parallel test
2727
scheduling and keep runtimes predictable.
2828
- Coverage reports (XML/Markdown/HTML) are produced by the nox `tests` session
2929
and stored under `coverage/py<version>/` (for example,
3030
`coverage/py3.12/coverage.xml`, `coverage/py3.12/coverage.md`,
3131
`coverage/py3.12/html/`).
3232

33+
## Code Quality Checklist
34+
35+
- When code changes are complete, or when asked to "check code quality", run
36+
this default sequence:
37+
- `uv run ruff format .`
38+
- `uv run ruff check .`
39+
- `uv run basedpyright`
40+
- Do not include pytest or nox runs in the default "code quality" request; treat
41+
runtime tests as a separate validation step.
42+
43+
## Test Validation Checklist
44+
45+
- Run tests separately from code quality checks:
46+
- `uv run pytest -n auto --maxschedchunk 2` (or a focused module when
47+
iterating)
48+
- For full compatibility before handoff/PR, run:
49+
- `uvx nox -s tests` (Python 3.10-3.14 matrix + coverage artifacts)
50+
- For class-function coverage gate validation (when relevant to client changes),
51+
run:
52+
- `uv run python scripts/check_class_function_coverage.py coverage/py<version>/coverage.json --fail-under 90 --class PdfRestClient --class AsyncPdfRestClient --class _FilesClient --class _AsyncFilesClient`
53+
- Always report:
54+
- files changed
55+
- tests/checks run and not run
56+
- why any checks were skipped
57+
3358
## Coding Style & Naming Conventions
3459

3560
- Target Python 3.10–3.14; use 4-space indentation and type hints for public
@@ -125,7 +150,9 @@
125150
a shared validation suite when multiple endpoints rely on the same input rules
126151
(e.g., `tests/test_graphic_payload_validation.py`).
127152
- Do not import from private modules (names beginning with an underscore) in
128-
tests or production code—expose any shared helpers via a public module first.
153+
production code. In tests, prefer public modules first; allow private-model
154+
imports only when necessary to validate request serialization or mock
155+
server-facing payload contracts that are not exposed publicly.
129156

130157
## Testing Guidelines
131158

@@ -134,7 +161,9 @@
134161
considered complete. Mirror the naming/structure used by the graphic
135162
conversion suites: one module per endpoint, parameterized success cases that
136163
enumerate all accepted literals, at least one invalid input that hits the
137-
server, and coverage for any request options surfaced on the client. If an
164+
server, and coverage for server-observable endpoint options. Validate
165+
`extra_query`/`extra_headers`/`extra_body`/`timeout` plumbing in unit tests
166+
(MockTransport) unless a live assertion depends on those options. If an
138167
endpoint cannot be exercised live, call that out explicitly in the PR
139168
description with the reason and the follow-up plan; otherwise reviewers should
140169
block the change. Treat this as a release gate on par with unit tests.

‎TESTING_GUIDELINES.md‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,15 +201,22 @@ iteration required.
201201
predictable. Use `pytest.fixture(scope="module"/"class")` and
202202
`pytest_asyncio.fixture` to cache uploaded PDFs/profiles for both transports.
203203
- **Sync + async parity:** Every live module should contain matching sync and
204-
async tests for success, customization, streaming, and invalid paths
205-
(compression levels, conversion options, file streaming helpers).
204+
async tests for success, streaming, and invalid paths (compression levels,
205+
conversion options, file streaming helpers). Treat transport-plumbing
206+
customization (`extra_query`, `extra_headers`, `extra_body`, `timeout`) as a
207+
unit-test responsibility unless the endpoint exposes server-observable
208+
behavior tied to those options.
206209
- **Enumerate literals:** Parameterize over every accepted literal (compression
207210
levels, `color_model`, `smoothing`, merge selectors, redaction presets). Each
208211
literal should hit the server once per transport.
209212
- **Optional arguments:** Exercise options such as custom output prefixes,
210213
diagnostics toggles, merge metadata, and URL uploads. Validate the server
211214
honors them (filenames start with the user-provided prefix, warnings appear
212215
when expected).
216+
- **Customization placement:** Verify request customization wiring in mocked
217+
tests (sync + async) for each endpoint. Live customization tests are optional
218+
and should be added only when they validate server-side behavior beyond client
219+
request assembly.
213220
- **Negative live cases:** Override JSON via `extra_body`/`extra_query` to
214221
bypass local validation and assert `PdfRestApiError` (or the exact server
215222
exception) surfaces—for example, sending an invalid compression literal or

0 commit comments

Comments
 (0)