diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000000..ec19cac1d3 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,52 @@ +## What this changes + + + +## Motivation + + + +## Testing + + + +--- + +### If this touches the Studio UI + +Studio changes are expected to leave the interface behaving the same way it did before. +Tick whichever applies, and delete the rest. + +- [ ] No user-visible change. The UI behaves identically. +- [ ] A deliberate UI difference, justified by a dramatic performance improvement. The + difference and its cost to the user are described above, with the measurement. +- [ ] The difference exists only off screen. Everything inside the viewport is unchanged. + +If you ticked the third box, confirm that selection, clipboard, native find-in-page, +printing and scroll geometry all still cover the whole conversation rather than the visible +window. Those are whole-document operations and the off-screen exemption does not reach +them. + +**Whichever box you ticked, run the parity suite and paste its verdict**, including the +number of action pairs compared and the concurrent null control's score on the same run. A +parity result without its null beside it is not evidence, because it gives no floor to judge +a difference against. + +``` +parity verdict: +pairs compared: +null control: +``` + +Two things the verdict does not tell you on its own, so say which one you are claiming: + +- The structural digest covers thread structure. It is blind to the sidebar and to computed + layout, and it never reads geometry or custom properties. A change confined to either will + pass it while being invisible to it. +- Visible-region parity is the check that supports the off-screen exemption. It is the one to + use for a windowed or deferred change, since a whole-document digest will report + differences by design and prove nothing. + +See [CONTRIBUTING.md](https://github.com/unslothai/unsloth/blob/main/CONTRIBUTING.md) for the +full rule. diff --git a/.github/workflows/workflow-trigger-lint.yml b/.github/workflows/workflow-trigger-lint.yml index 86499d0bf2..a0dd2a083a 100644 --- a/.github/workflows/workflow-trigger-lint.yml +++ b/.github/workflows/workflow-trigger-lint.yml @@ -154,6 +154,7 @@ jobs: tests/studio/test_main_runs_survive_merge_bursts.py \ tests/studio/test_pester_bootstrap_hardening.py \ tests/studio/test_playwright_suites_run_in_ci.py \ + tests/studio/test_pull_request_template_links.py \ tests/studio/test_sdk_installs_are_major_bounded.py \ tests/studio/test_short_job_absorption.py \ tests/studio/test_smoke_workflows_share_one_script.py \ diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6eb8d1bc6e..071caf2912 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -33,3 +33,43 @@ Thank you so much for reading and we hope you have lots of fun using Unsloth! - Keep PRs focused on a single change - Include a concise description and motivation - Link related issues when applicable + +### Changes to Studio's UI + +A pull request that touches the Studio frontend must leave the interface behaving the same +way it did before. Reviewers should be able to assume that a change described as a +performance fix is a performance fix and nothing else. + +There are two exemptions. + +1. **A dramatic performance improvement can justify a deliberate UI difference.** State the + difference plainly in the PR body, say what it costs the user, and attach the measurement + that justifies it. A difference that is not called out is a regression, however fast the + PR is. +2. **A difference that exists only off screen is fine.** Rendering only what is visible is an + accepted technique, not a parity violation. Windowing, deferring off-screen work and + unmounting rows the user cannot see are all allowed, provided everything inside the + viewport is identical to what it was before. + +The second exemption is narrower than it sounds, and the boundary is where these changes go +wrong in practice: + +- An element that is partly on screen is visible, and so is one that scrolls into view during + an interaction. +- Selection, clipboard, native find-in-page and printing are whole-document operations. A + user pressing Ctrl+A, Ctrl+C or Ctrl+F is asking about the conversation, not about the + viewport, so off-screen content still has to participate. Truncating a copied thread is a + data-loss bug, not an off-screen rendering difference. +- Scroll geometry is visible. A scrollbar that no longer describes the length of the thread + is a UI change even though the thing that shrank was off screen. + +If you are changing what the thread renders, measure at the 100K rung or larger against a +concurrent control, and say which of the two exemptions you are relying on, or that you are +relying on neither. + +Run the parity suite either way and put its verdict in the PR, with the number of action +pairs compared and the concurrent null control's score on the same run. Without the null +there is no floor, so a clean-looking verdict cannot be distinguished from a suite that +compared nothing. Say which claim the verdict supports: the structural digest covers thread +structure and is blind to the sidebar and to computed layout, while visible-region parity is +the check that supports the off-screen exemption. diff --git a/tests/studio/test_pull_request_template_links.py b/tests/studio/test_pull_request_template_links.py new file mode 100644 index 0000000000..e2642b73cc --- /dev/null +++ b/tests/studio/test_pull_request_template_links.py @@ -0,0 +1,125 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +"""Every link in the PR template has to survive being pasted into a pull request body. + +GitHub copies the template verbatim into the PR description, rendered at +`///pull/`, so a DOCUMENT-relative target resolves from THAT path: +`../CONTRIBUTING.md` becomes `///CONTRIBUTING.md`, which 404s. A ROOT-relative +target is safe, since the leading slash discards the PR path and resolves against the host. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +TEMPLATE = Path(__file__).resolve().parents[2] / ".github" / "PULL_REQUEST_TEMPLATE.md" + +# [text](target), ignoring images. The <> wrapper is not part of the URI (CommonMark 0.31.2 +# section 6.3), so keeping the brackets would read an absolute `` as relative. +LINK = re.compile(r"(?\s]+)>?") +# [label]: target -- reference definitions resolve against the same wrong base, so a guard reading +# only the inline form would pass a template whose links all 404. The optional <> wrapper and the +# trailing title must not end up in the target. +REF_DEF = re.compile(r"^[ ]{0,3}\[[^\]]+\]:[ \t]*\s]+)>?", re.MULTILINE) + + +def _targets(text: str) -> list[str]: + return LINK.findall(text) + REF_DEF.findall(text) + + +def _resolves_off_the_pr_path(target: str) -> bool: + """True when GitHub would resolve `target` from `///pull/`. + + A leading slash is NOT such a target: it resolves against the host, landing on the same + page as the absolute URL. + """ + if target.startswith(("http://", "https://", "mailto:", "#", "/")): + return False + return True + + +def test_template_exists() -> None: + assert TEMPLATE.is_file(), f"{TEMPLATE} is missing" + + +def test_no_relative_link_targets_in_the_pr_template() -> None: + relative = [ + target + for target in _targets(TEMPLATE.read_text(encoding = "utf-8")) + if _resolves_off_the_pr_path(target) + ] + assert not relative, ( + f"{len(relative)} relative link target(s) in .github/PULL_REQUEST_TEMPLATE.md: " + f"{relative}. The template is rendered inside a PR body at /owner/repo/pull/N, not " + "from .github/, so a document-relative target resolves off the repository tree and " + "404s. Use the absolute https://github.com/... blob URL, or a root-relative " + "/owner/repo/blob/... target, either of which ignores the PR path." + ) + + +@pytest.mark.parametrize( + "target,relative", + [ + ("../CONTRIBUTING.md", True), + ("CONTRIBUTING.md", True), + ("./docs/CONTRIBUTING.md", True), + # Root-relative: the leading slash drops the PR path, so rejecting it would fail CI + # on a valid link. + ("/unslothai/unsloth/blob/main/CONTRIBUTING.md", False), + ("https://github.com/unslothai/unsloth/blob/main/CONTRIBUTING.md", False), + ("#testing", False), + ], +) +def test_the_guard_classifies_targets_the_way_github_resolves_them( + target: str, relative: bool +) -> None: + """The guard itself, against the shapes it has to separate. + + Without this the test above also passes for a template with no links at all. + """ + found = _targets(f"See [CONTRIBUTING.md]({target}) for the full rule.") + assert found == [target] + assert _resolves_off_the_pr_path(target) is relative + + +@pytest.mark.parametrize( + "target", + [ + "https://github.com/unslothai/unsloth/blob/main/CONTRIBUTING.md", + "/unslothai/unsloth/blob/main/CONTRIBUTING.md", + "../CONTRIBUTING.md", + ], +) +def test_the_scan_unwraps_angle_bracketed_inline_destinations(target: str) -> None: + """`[text]()` is the same link as `[text](target)`: the brackets wrap the + destination and are not part of the URI. + """ + found = _targets(f"See [CONTRIBUTING.md](<{target}>) for the full rule.") + assert found == [target] + assert _resolves_off_the_pr_path(target) is _resolves_off_the_pr_path(found[0]) + + +@pytest.mark.parametrize( + "body, caught", + [ + ("See [guidelines][contrib].\n\n[contrib]: ../CONTRIBUTING.md\n", True), + ("See [guidelines][contrib].\n\n[contrib]: <../CONTRIBUTING.md>\n", True), + ('See [g][c].\n\n[c]: ../CONTRIBUTING.md "Contributing"\n', True), + ("[c]: https://github.com/unslothai/unsloth/blob/main/CONTRIBUTING.md\n", False), + ("See [inline](../CONTRIBUTING.md).\n", True), + ("![shot](../docs/a.png)\n", False), + ], +) +def test_the_scan_reads_reference_definitions_as_well_as_inline_links(body, caught) -> None: + """A reference-style link resolves against the same wrong base as an inline one, so a + template that moved to `[guidelines][contrib]` would have passed while every rendered + link 404'd. + """ + relative = [ + t for t in _targets(body) if not t.startswith(("http://", "https://", "mailto:", "#")) + ] + assert bool(relative) is caught, f"targets found: {_targets(body)}"