Add REST API contract summaries to PR descriptions#3789
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - The summary generation is simple and the focused tests pass, but the PR-body updater has a user-visible edge case around the protected HUMAN section.
[CRITICAL ISSUES]
- [.github/scripts/update_pr_body_with_rest_api_summary.py, Line 22] Data Ownership / Compatibility:
_summary_bounds()searches the entire PR body for the first## Summary. If the human-authored section contains its own## Summary, the workflow inserts the generated block into the wrong part of the description instead of the AGENT summary, violating the template's "Do not edit the HUMAN section" contract. Scope the search to the AGENT section (or an explicit marker after the `---
AGENT:boundary) before locating## Summary`.
[TESTING GAPS]
- Add a regression test where the HUMAN section contains a
## Summaryheading and the AGENT section also contains## Summary; the generated block should land in the AGENT summary and the HUMAN text should remain unchanged.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is workflow/tooling-only and does not alter SDK runtime behavior, but it grants the workflowpull-requests: writeand automatically patches PR descriptions. The current section-selection bug can mutate or insert into protected author-controlled text on same-repository PRs, so it should be fixed before merge.
VERDICT:
❌ Needs rework: Fix the PR-body section targeting so the automation cannot touch the HUMAN section.
KEY INSIGHT:
The core data-ownership boundary is the HUMAN/AGENT split, but the updater currently treats the whole PR body as one undifferentiated string.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
|
||
|
|
||
| def _summary_bounds(body: str) -> tuple[int, int] | None: | ||
| summary_match = SUMMARY_HEADING_RE.search(body) |
There was a problem hiding this comment.
🟠 Important: This searches from the top of the entire PR body, so the first ## Summary wins even if it belongs to the protected HUMAN section. For example, a human-authored ## Summary before the `---
AGENT:` boundary causes the generated block to be inserted before the AGENT summary instead of inside it. That violates the PR template's “Do not edit the HUMAN section” contract. Please scope the search to the AGENT section (or another explicit generated-content boundary) before looking for the summary heading, and add a regression test for that layout.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
The REST API contract summary flow works end-to-end in local workflow-style execution, with one minor side effect around PR-body newline normalization.
Does this PR achieve its stated goal?
Yes. The new generator runs on the PR branch, emits no summary for this PR because the public /api/** contract is unchanged, and emits a concise diff when I temporarily added a real public REST endpoint. The PR-body updater inserted that generated block inside ## Summary and removed it again when the summary became empty, while preserving the visible HUMAN text.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully |
| CI Status | ✅ gh pr checks showed 36 passing checks and 15 skipped checks |
| Functional Verification |
Functional Verification
Test 1: REST API contract summary generator
Step 1 — Establish baseline without the PR feature:
Ran the new CLI path from a detached worktree at the PR base SHA:
python: can't open file '/tmp/software-agent-sdk-base-3789/.github/scripts/generate_agent_server_rest_api_contract_summary.py': [Errno 2] No such file or directory
base_cli_exit=2
This shows the PR is adding a new workflow-facing CLI entrypoint that did not exist on main.
Step 2 — Apply the PR's changes:
Used the checked-out PR branch at 01822c13634427543ba881dbc1306f1607cc7389 after make build.
Step 3 — Run with the PR in place against the actual PR base:
Ran:
uv run --with packaging python .github/scripts/generate_agent_server_rest_api_contract_summary.py --base-ref 9a3721a25dd8b8cbc742350fe3b30c2d3d130807 --output /tmp/rest-api-contract-summary-pr3789.md
summary_bytes=0
summary_preview=<>
This shows the generator completed successfully and correctly produced an empty summary for this PR, which does not itself change the public REST API contract.
Step 4 — Exercise a real public REST API addition:
Temporarily added a QA-only GET /api/tools/qa-contract-summary endpoint to the local working tree, then reran the same generator command:
summary_bytes=436
### REST API contract changes
Compared with base OpenAPI `9a3721a25dd8` for public `/api/**` paths.
```diff
--- base public OpenAPI
+++ head public OpenAPI
@@ -49,0 +50 @@
+operation GET /api/tools/qa-contract-summary operationId=qa_contract_summary_probe_api_tools_qa_contract_summary_get
@@ -310,0 +312 @@
+response GET /api/tools/qa-contract-summary 200 application/json schema=type="object" additionalProperties=type="string"
```
This confirms the generator surfaces real public REST API additions as concise PR-description diff content. The temporary endpoint was restored afterward.
Test 2: PR body updater insertion and removal
Step 1 — Establish baseline body and empty summary:
Fetched the actual PR body with gh api repos/OpenHands/software-agent-sdk/pulls/3789 --jq .body, then ran the updater with the empty summary produced for the current PR:
body_update_result=changed
second_update_result=unchanged
This revealed the minor newline-normalization issue listed below: visible content was unchanged, but the first local update rewrote line endings.
Step 2 — Apply a generated summary block:
Used the non-empty summary from the temporary real endpoint addition and ran:
python .github/scripts/update_pr_body_with_rest_api_summary.py --body-file /tmp/pr-body-3789.md --summary-file /tmp/rest-api-contract-summary-added-endpoint.md --output /tmp/updated-pr-body-with-summary.md
Observed:
has_start_marker= True
has_end_marker= True
marker_after_summary= True
marker_before_issue_number= True
human_text_present= True
The generated block landed inside ## Summary, before ## Issue Number, and the visible HUMAN text remained present.
Step 3 — Remove stale summary when no contract changes remain:
Ran the updater again with the empty summary file:
has_start_marker= False
has_temp_endpoint_summary= False
summary_bullets_present= True
human_text_present= True
This confirms stale generated blocks are removed while the existing summary bullets and visible HUMAN text remain.
Issues Found
- 🟡 Minor: Running the updater against the actual PR body with an empty generated summary still changed the output once because CRLF line endings were normalized to LF. This can cause an unnecessary PR-body PATCH even when there is no REST API contract block to add/remove; visible content was preserved.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
Final verdict: PASS WITH ISSUES
|
|
||
| def main() -> int: | ||
| args = parse_args() | ||
| body = args.body_file.read_text() |
There was a problem hiding this comment.
🟡 Suggestion: In functional QA, running the updater against the actual PR body with an empty generated summary still produced a changed output because read_text()/write_text() normalized the PR body's CRLF line endings to LF. That means the workflow can PATCH a PR description even when there is no REST API summary block to insert or remove. Consider preserving the original newlines or comparing normalized content before deciding to update the PR body.
2 participants
HUMAN:
We all know API design matters, for usability, maintainability and all; the funny thing we need to grapple with, is that it’s possible that API design is one of the (few?) things where humans need to think things over.
This PR proposes adding a concise view of API changes to the PR descriptions. Even when the incompatible REST API checks workflow succeeds, it means additions, and it’s good to know to add the right things to avoid changing them too soon.
AGENT:
This PR was created by an AI agent (OpenHands) on behalf of the user.
Why
Recent agent-server REST API PRs (#3770, #3784, and #3788) made API-review details hard to see at a glance from the normal PR description. The existing REST API workflow already generates and compares OpenAPI contracts; this adds a concise PR-description summary for additions, removals, and modifications against the PR base.
Summary
/api/**OpenAPI surface into operation/schema lines and emits a concise unified diff against the PR base SHA.## Summarysection without touching the HUMAN section.Issue Number
N/A
How to Test
make builduv run pytest tests/cross/test_agent_server_rest_api_contract_summary.pyuv run pytest tests/cross/test_check_agent_server_rest_api_breakage.py tests/cross/test_agent_server_rest_api_contract_summary.pyuv run --with packaging python .github/scripts/generate_agent_server_rest_api_contract_summary.py --base-ref origin/main --output /tmp/rest-api-contract-summary.mduv run pre-commit run --files .github/scripts/generate_agent_server_rest_api_contract_summary.py .github/scripts/update_pr_body_with_rest_api_summary.py .github/workflows/agent-server-rest-api-breakage.yml tests/cross/test_agent_server_rest_api_contract_summary.pyVideo/Screenshots
N/A
Type
Notes
@enyst can click here to continue refining the PR