docs: encourage .pr/ HTML design doc + htmlpreview link for non-trivial PRs - #4371
Conversation
…al PRs Add a soft guideline mirroring OpenHands/OpenHands: for non-trivial PRs, commit a self-contained HTML design doc under the temporary .pr/ directory (auto-removed on approval) and link it in the PR description via htmlpreview so reviewers see the code/API design and before/after at a glance. - PR template: new optional 'Design Doc' section with the htmlpreview link shape. - CONTRIBUTING.md: a 'Design doc for non-trivial PRs' section, emphasizing the before/after of the interface and the compatibility impact this repo cares about. Co-authored-by: smolpaws <engel@enyst.org>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Docs-only PR adding an optional, soft-encouraged guideline: commit a self-contained HTML design doc under the temporary .pr/ directory and link it via htmlpreview so reviewers can see the interface before/after and compatibility impact at a glance. The addition lands in both .github/PULL_REQUEST_TEMPLATE.md (a ## Design Doc section) and CONTRIBUTING.md (a "Design doc for non-trivial PRs" section).
The guideline is well-scoped: explicitly optional, correctly scoped to non-trivial PRs (public API / agent-loop / migration), and it points at the right compatibility lens (additive / breaking / behind a flag) this repo reviews for. The htmlpreview link shape is consistent between the two files, and the htmlpreview only works for public repos and self-contained pages caveat is stated in both places.
Findings
One material accuracy issue (detailed inline): the auto-removal guarantee is stated unconditionally, but .github/workflows/pr-artifacts.yml only auto-removes .pr/ for same-repository PRs (head.repo.full_name == github.repository). Fork PRs get no auto-cleanup, yet the link shape points contributors at <your-fork>. For a fork PR approved and merged without manual removal, .pr/ can land in main, so "it never lands in main" is not guaranteed there. The repo's own AGENTS.md already documents this fork caveat ("For fork PRs: You must manually remove .pr/ before the PR can be merged"); the new user-facing docs should align by noting fork PRs require manual removal.
Risk assessment
Low risk. No code, no tests, no config, no security surface. The only issue is a documentation accuracy gap that could mislead fork-based contributors into assuming .pr/ is always cleaned up. Recommend adding a one-line fork caveat; otherwise this is good to merge.
Correctness / security / tests
- Correctness: No executable code; claims verified against
.github/workflows/pr-artifacts.yml(auto-removal is same-repo only) and the existing.pr/convention. - Security: No concerns — docs only, no code execution, no secrets, no external network calls introduced.
- Tests: Appropriately none for a docs-only change.
- Style: Markdown is consistent with the surrounding files; no lint concerns.
The pr-artifacts workflow cleans up .pr/ on approval only for same-repository PRs, and removes it from the base branch right after merge for fork PRs. Update the design-doc guidance and PR template to describe both paths accurately. Co-authored-by: openhands <openhands@all-hands.dev>
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
Superseded: this was an OpenHands Cloud Automation smoke test; the actual review is at the next comment. View conversation. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
2 similar comments
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
✅ 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.
🟢 Good taste — small, focused, docs-only and accurate.
Verified the docs against .github/workflows/pr-artifacts.yml: cleanup-on-approval is gated on same-repo (it only fires when head.repo.full_name == github.repository), and cleanup-after-merge handles fork PRs by checking out the base branch and removing .pr/ after merge. Both paths match the wording in the PR template and CONTRIBUTING.md.
The latest commit (ba8192ae) addresses the prior review thread by splitting out the same-repo vs fork cleanup behavior explicitly — good catch on that one. The htmlpreview link shape is correct, and the docs stay aligned with the broader OpenHands/OpenHands guideline PR.
Nothing to block. Approving.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
3 participants
HUMAN:
Docs-only change, the SDK counterpart to OpenHands/OpenHands#16316. Reviewed the rendered template and CONTRIBUTING section locally; htmlpreview link shape verified against the .pr/ convention already wired in this repo.
AGENT:
Why
Reviewers of a non-trivial SDK PR often reconstruct the design from the diff alone — slow, and it is exactly where the compatibility risks this repo cares about hide (Python API + agent-server REST/WebSocket surface). This adds a soft, optional guideline: commit a self-contained HTML design doc under the temporary
.pr/directory and link it via htmlpreview so reviewers see the code/API design and before/after at a glance. The.pr/directory is already auto-removed on approval (.github/workflows/pr-artifacts.yml).This is the counterpart to the OpenHands/OpenHands guideline PR, same process, so both repos read consistently.
Summary
## Design Docsection with the htmlpreview link shape.Issue Number
N/A (docs; companion to OpenHands/OpenHands#16316)
How to Test
Docs only — nothing to run. Read
.github/PULL_REQUEST_TEMPLATE.md(new## Design Docblock) andCONTRIBUTING.md("Design doc for non-trivial PRs"). The htmlpreview link shape is:Companion PRs: OpenHands/OpenHands#16316 (same guideline) and OpenHands/extensions#451 (the
pr-design-docskill that generates the page).Type
Notes
Soft-encouraged, not enforced. htmlpreview works only for public repos and self-contained pages.
Co-authored-by: smolpaws engel@enyst.org