Repository navigation
feat(pr_review): track audit checklist items in a JSON sidecar - #1536
VLukiantsev merged 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a machine-readable JSON sidecar alongside the markdown audit checklists and reports to serve as the source of truth for PR review item statuses. It updates the audit generation tool to output these sidecars and enhances the verification tool to parse them, support a markdown fallback, and provide a new '--mark-posted' flag. The review feedback suggests improving the robustness of the sidecar parser by coercing string-serialized line numbers and thread IDs to integers, and ensuring that markdown checkbox statuses for re-review threads are updated consistently alongside the JSON sidecar.
Agents hand-edited item statuses in pr_review_audit_checklist.md (Approved -> Posted) with exact-match edits that often failed. review_pr_verify_posted.py parsed only status tables and '### Thread ID' blocks, so a checklist written with '#### #N' headings yielded 0 items and verification "passed" with a warning. It also marked every approved item Posted, even items it had just reported as missing. - review_pr_generate_audit.py writes a '.json' sidecar next to each audit artifact: an empty item list in initial mode, and one item per root thread (action null) in re-review mode. - review_pr_verify_posted.py reads items from the sidecar (markdown fallback), exits non-zero when it finds no approved items, records only verified items as Posted, and gains '--mark-posted ITEM_ID...' to record posted items in the sidecar and markdown without hand edits. - The xmc_review_pr skill and tools README describe the sidecar flow. Existing markdown-only checklists still verify through the fallback. TAG=agy CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53
The verifier fetched /pulls/{n}/comments without pagination, so it only
saw GitHub's first page (30 comments). On larger PRs, approved comments
that were actually posted were reported as missing.
The private REST page loop in review_pr_generate_audit.py moves to
github_utils.list_all_pages so both tools share one implementation, and
the verifier uses it for live comments. Tests cover the helper and a
match on the second page of live comments.
TAG=agy
CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53
The schema_version key was written to the audit sidecar but never read or validated by review_pr_verify_posted.py. The sidecar is ephemeral (same checkout, same session), so the version was dead metadata. The reader still fails loudly when the sidecar has no 'items' list, now covered by a test. TAG=agy CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53
Addresses review feedback: agents often write "line": "15" in the audit sidecar, which was silently skipped. A small _as_positive_int() helper now accepts integer strings and still rejects bools, floats, and non-positive values; malformed items stay skipped, and zero approved items still fails loudly. TAG=agy CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53
f5fdc00 to
7f95986
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces machine-readable JSON sidecars to serve as the source of truth for PR review audits, updating the agent's skills, documentation, and tools. Key changes include adding a paginated REST helper list_all_pages in github_utils.py, generating JSON sidecars in review_pr_generate_audit.py, and refactoring review_pr_verify_posted.py to verify posted comments using these sidecars and support a new --mark-posted flag. The review feedback is highly constructive, pointing out an issue where the markdown file is not loaded when a JSON sidecar is passed directly, and suggesting a clearer error message path when no approved items are found.
Addresses review feedback on review_pr_verify_posted.py: - When -c points at the .json sidecar, the sibling .md is loaded if it exists, so markdown table rows are still updated on Posted and the markdown RESOLVE checkbox still applies, as with -c on the .md. - The zero-items error names the sidecar file when the sidecar is the item source, instead of the markdown path. TAG=agy CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53
Summary
Audit checklist items are now tracked in a JSON sidecar next to each audit markdown artifact.
review_pr_verify_posted.pyreads the sidecar (with a markdown fallback), fails when it finds no approved items, and gains--mark-posted ITEM_ID..., so agents no longer hand-edit statuses.Motivation & Context
In review sessions, agents updated statuses in
pr_review_audit_checklist.md(Approved → Posted) with exact-match text edits that often failed. The verifier only parsed status tables and### Thread IDblocks. A checklist written with#### #Nheadings gave 0 items, and the tool returned a warning that looked like success. It also marked every approved itemPosted, even items it had just reported as missing.Key Changes
review_pr_generate_audit.pywrites<artifact>.json(PR identity,head_sha,items). Initial mode starts with no items. Re-review mode lists each root thread with"action": null. The output JSON now includessidecar_file.review_pr_verify_posted.py:HandledError) on zero approved items instead of returningstatus: warning.Posted, in the sidecar and the markdown table.--mark-postedrecords approved items as posted without contacting GitHub. Unknown or unapproved IDs are rejected.#1and1are treated as the same ID.-c.review_pr_generate_audit.pymoves togithub_utils.list_all_pages, used by both tools (one implementation, no new pagination code)..agents/skills/xmc_review_pr/SKILL.mdandtools/README.mddescribe the sidecar flow: write items by rewriting the JSON, mark them posted via the tool, never edit statuses by hand.--mark-posted(success, unknown/unapproved IDs), invalid JSON, item filtering, and sidecar generation for both modes.Reuse:
tool_utils.HandledError/run_tool, the existing markdown parsers and checks in the verifier, and the existing REST page loop (moved togithub_utils.list_all_pages, not rewritten).tool_utils.pyis not touched.Assumptions
.jsonsuffix (pr_review_audit_checklist.json). Regenerating the audit overwrites it, just like the markdown.--mark-postedonly accepts approved items (status Approved/Confirmed/Posted, or re-review threads with a RESOLVE/RESPOND action), so marking cannot skip the confirmation gate.line/thread_idaccept integer strings ("15"), since agents often write them that way. Bools, floats, and non-positive values are rejected, and items with malformed values are skipped. Zero approved items still fails loudly.tools/pr_review/wait_for_ci.pyalready haspositive_int()(an argparse type), and this PR adds_as_positive_int()for JSON values. Once PR fix(tools): harden run_command and adopt it in tools/lint #1532 (which editstools/common/tool_utils.py) merges, both should be consolidated into one shared helper intool_utils.py.tool_utils.pyis deliberately not touched here.Verification & Testing
bazel test //tools/pr_review/..., full./.githooks/pre-commit(Ruff, mypy, agent doc path check, affected tests) as the git hook for every commit.list_all_pages. Diff vs. parent: 298 prod / 294 test / 21 doc lines.Related Issues / Bugs
Stack layer 4/4 (pr_review hardening). N/A
TAG=agy
CONV=fdd0f472-8c78-4d10-947b-f2191f7c5f53