docs: encourage .pr/ HTML design doc + htmlpreview link for non-trivial PRs - #4371
docs: encourage .pr/ HTML design doc + htmlpreview link for non-trivial PRs#4371smolpaws wants to merge 1 commit into
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.
| state the compatibility impact (additive, breaking, or behind a flag). | ||
| 2. Commit it under the temporary **`.pr/`** directory, e.g. `.pr/design.html`. This directory is | ||
| for PR-only artifacts and is **removed automatically when the PR is approved** | ||
| (`.github/workflows/pr-artifacts.yml`), so it never lands in `main`. |
There was a problem hiding this comment.
The auto-removal is not unconditional. cleanup-on-approval in .github/workflows/pr-artifacts.yml is gated on github.event.pull_request.head.repo.full_name == github.repository, so only same-repository PRs get .pr/ removed on approval. Fork PRs (which the <your-fork> link shape explicitly targets) are not cleaned up automatically, so for those "it never lands in main" only holds if the author manually removes .pr/ before merge — otherwise it can be merged into main.
The repo's own AGENTS.md already notes this ("For fork PRs: You must manually remove .pr/ before the PR can be merged"). Consider adding a short caveat here and in the PR template, e.g. "(fork PRs require manual removal before merge; the auto-cleanup runs for same-repo PRs only)".
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