Skip to content

Add lightweight PR review scaffold#830

Merged
huangruiteng merged 4 commits into
mainfrom
codex/pr-review-rich-cards-20260628
Jun 28, 2026
Merged

Add lightweight PR review scaffold#830
huangruiteng merged 4 commits into
mainfrom
codex/pr-review-rich-cards-20260628

Conversation

@huangruiteng

@huangruiteng huangruiteng commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • Simplify /loopx-pr-review into a lightweight, generic PR review scaffold instead of trying to write the review itself.
  • Add explicit review_groups.unmerged and review_groups.merged so agentloop follows the user's intended open/unmerged vs merged-review flow.
  • Add a slash-command agent_contract and loopx-project skill wording that requires running loopx pr-review first; manual gh calls are only for per-PR deep reading after the CLI packet selects a PR.
  • Keep the five-block template intentionally blank: 动机改动思路具体改动对主干的风险我的整体评价.

Validation

  • python3 examples/pr-review-command-smoke.py
  • python3 examples/slash-command-catalog-smoke.py
  • python3 examples/bootstrap-command-pack-smoke.py
  • python3 -m py_compile loopx/pr_review.py loopx/slash_commands.py loopx/cli_commands/pr_review.py
  • git diff --check
  • loopx check --scan-path loopx/pr_review.py --scan-path loopx/slash_commands.py --scan-path skills/loopx-project/SKILL.md --scan-path examples/pr-review-command-smoke.py --scan-path examples/slash-command-catalog-smoke.py --scan-path examples/bootstrap-command-pack-smoke.py --scan-path docs/reference/protocols/pr-review-command-v0.md
  • python3 -m loopx.cli pr-review --fixture examples/fixtures/pr-review.public.json --limit 5

Boundary

  • Generic repository behavior only; no LoopX-specific hot-path special casing.
  • No review text is fabricated by the tool; agentloop must read PR body/diff before filling the blank template.
  • Public PR metadata and public docs/smokes only; no private links, raw logs, credentials, or local paths.

@huangruiteng huangruiteng changed the title Refine PR review guided cards Add lightweight PR review scaffold Jun 28, 2026
@huangruiteng

Copy link
Copy Markdown
Owner Author

Findings: none from this review pass.

Open questions / assumptions: I reviewed the current PR head, including the final simplification to a blank five-block template. The docs, fixture, smoke, and implementation now agree that metadata risk is only queue-ordering context and the agent must read the real PR body/diff before filling review content.

Validation performed:

  • python3 examples/pr-review-command-smoke.py passed on the current PR head.
  • Fixture CLI output contains review_template and metadata_risk_hint, and no longer emits filled guided_review_card / main_regression_analysis content by default.

Merge decision: hold for normal reviewer/branch-protection flow; no protocol/CLI blocker found in this pass.

@huangruiteng

Copy link
Copy Markdown
Owner Author

Review result: hold / changes needed. GitHub will not let me formally request changes on my own PR, so leaving the review as a PR comment.

Findings:

  • [P1] loopx/pr_review.py:437 / loopx/pr_review.py:668 replaces the shipped main_regression_analysis payload with a metadata_risk_hint and a blank review template. That would regress /loopx-pr-review after Add main risk analysis to pr review #829: the command would no longer give each PR concrete main-regression classes, bug risks, or verification focus, which is exactly the material a human reviewer needs for merged+open PR review. I verified the fixture output on this branch has has_main_regression_analysis=false and all five template section contents empty. Please keep the five-block scaffold if useful, but preserve or reintroduce generated main_regression_analysis (or equivalent filled risk fields) in both JSON and markdown.

  • [P2] docs/reference/protocols/pr-review-command-v0.md:16 reframes the command as providing a blank template that "agentloop fills". That may be true as a workflow layer, but the command contract should still expose enough concrete review material for a human/controller packet without relying on chat memory or an extra hidden agent pass. Please document the split as: CLI supplies metadata, concrete risk analysis, evidence commands, and a fillable/fillable-by-agent review scaffold.

Validation performed:

  • python3 examples/pr-review-command-smoke.py passed
  • python3 -m py_compile loopx/pr_review.py examples/pr-review-command-smoke.py passed
  • git diff --check origin/main...HEAD passed
  • loopx check --scan-path loopx/pr_review.py --scan-path examples/pr-review-command-smoke.py --scan-path examples/fixtures/pr-review.public.json --scan-path docs/reference/protocols/pr-review-command-v0.md passed with only the pre-existing duplicate-index warning

Merge decision: hold. The implementation is cleanly validated, but it currently regresses the richer PR review contract on main.

@huangruiteng

Copy link
Copy Markdown
Owner Author

Findings: none on the current owner-directed version.

Open questions / assumptions: Earlier hold feedback on this PR asked for richer generated risk analysis, but the product direction was later changed explicitly: /loopx-pr-review should be a generic PR-list + blank five-block review scaffold, and agentloop should read the real PR body/diff before filling review content. I reviewed the current head against that newer contract.

Validation performed:

  • python3 examples/pr-review-command-smoke.py
  • python3 examples/slash-command-catalog-smoke.py
  • python3 examples/bootstrap-command-pack-smoke.py
  • python3 -m py_compile loopx/pr_review.py loopx/slash_commands.py loopx/cli_commands/pr_review.py
  • git diff --check origin/main...HEAD
  • loopx check --scan-path ... on the touched runtime/docs/smoke/fixture files; only the pre-existing duplicate-index warning remains.

Merge decision: self-merge authorized by owner. This is a single-purpose PR review command contract change, public-safe, validated, and not Lark Kanban related.

@huangruiteng

Copy link
Copy Markdown
Owner Author

Admin-bypass merge audit note.

Owner explicitly authorized admin-bypass after normal gh pr merge --squash --delete-branch was blocked by base branch policy (mergeStateStatus=BLOCKED, mergeable=MERGEABLE, reviewDecision=REVIEW_REQUIRED).

Focused validation rerun immediately before merge:

  • python3 examples/pr-review-command-smoke.py
  • python3 examples/slash-command-catalog-smoke.py
  • python3 examples/bootstrap-command-pack-smoke.py
  • python3 -m py_compile loopx/pr_review.py loopx/slash_commands.py loopx/cli_commands/pr_review.py
  • git diff --check origin/main...HEAD

Scope remains the generic /loopx-pr-review CLI/slash/skill/docs/smoke contract: CLI-first grouped PR queues plus a blank five-block review scaffold for agentloop to fill from real PR evidence.

@huangruiteng huangruiteng merged commit 613a3c5 into main Jun 28, 2026
@huangruiteng huangruiteng deleted the codex/pr-review-rich-cards-20260628 branch June 28, 2026 05:21
@huangruiteng

Copy link
Copy Markdown
Owner Author

Follow-up note: #830 was already merged before the main-risk rework could land on that PR branch. I created follow-up PR #835 to restore concrete main_regression_analysis while keeping the blank five-block scaffold from #830: #835

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