MagicBeanDev/Longshot

Watch

Fork 48

docs(forge): a review that asks for a blocking change requests changes #135

Merged

HumanBean merged 1 commit from HumanBean/Longshot:docs/review-verdict into main

2026-11-13 06:13:08 +00:00

HumanBean commented

2026-11-13 06:07:59 +00:00

Owner

Why

forge.py pr review can already request changes, but nothing says when to. A review that asks for a fix before the merge could go out as a comment or an approval, and nothing on the PR would show it as blocked.

What changed

docs: docs/forge.md, "Reviewing a pull request", says how to pick the verdict. --request-changes is for a change that has to land before the merge: a bug, a broken rule from CONTRIBUTING.md, a failing check. That applies to a WIP PR too. --comment is for questions and suggestions. --approve is for a PR that can merge as it is, with nits allowed as inline comments.
forge: in scripts/forge.py, the module notes say the same, and so do the pr review description and the help for --approve, --comment and --request-changes. Behaviour is unchanged.

Flags and settings

None

Measurements

main: d40506c5 (2026-11-13)
this PR: docs/review-verdict @ c57cbf58
Hardware: Linux (CachyOS), Python 3; no GPU involved
Scene: none: docs and a command's help text, nothing in the game
Method: tests/test_forge_pr.py; python -I scripts/forge.py pr review --help; python -I -B tests/run_all.py on main and on this PR
How to reproduce: python -I scripts/forge.py pr review --help

	main @ `d40506c5`	this PR @ `c57cbf58`
`pr review --help` names when to request changes	no	yes
`tests/test_forge_pr.py`	25 pass	25 pass
`tests/run_all.py`	461 passed, 7 failed, 69 skipped	461 passed, 7 failed, 69 skipped

$ python -I -B tests/test_forge_pr.py
Ran 25 tests in 0.158s
OK
$ python -I -B tests/run_all.py
461 passed, 7 failed, 69 skipped in 60 s (linux, 24 at once)

Testing

Tested:

tests/test_forge_pr.py on Linux: all 25 pass.
pr review --help shows the new help text.
python -I -B tests/run_all.py on Linux: 461 passed, 7 failed, 69 skipped. The same 7 fail on main on this PC, which comes from this PC's environment: test_bake_portability, test_builder_log_read_only, test_program_cache_cleanup, test_control, test_longshot_ctl, test_program_cache_one_variant, test_vram_budget_row.

Not tested:

Nothing in the game: the change is docs and help text only.

Needs other testers:
None

Screenshots

Nothing on screen

Risk and rollback

None at runtime: only text changes. Reverting the commit is enough.

Reviewer notes

The CLI doesn't enforce the rule, because whether a change is blocking is the reviewer's call.

Issues

None

Checklist

For everyone

One topic.
The title is a Conventional Commit (type(scope): description).
Rebased on current main.
python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
Tested in the game, or what wasn't is named under Testing.
Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
No game files, dumps, build output or generated files.
third_party/ untouched (a new folder with a PROVENANCE.md instead).
The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
Docs updated.

🤖 Generated with Claude Code

## Why `forge.py pr review` can already request changes, but nothing says when to. A review that asks for a fix before the merge could go out as a comment or an approval, and nothing on the PR would show it as blocked. ## What changed - docs: `docs/forge.md`, "Reviewing a pull request", says how to pick the verdict. `--request-changes` is for a change that has to land before the merge: a bug, a broken rule from CONTRIBUTING.md, a failing check. That applies to a WIP PR too. `--comment` is for questions and suggestions. `--approve` is for a PR that can merge as it is, with nits allowed as inline comments. - forge: in `scripts/forge.py`, the module notes say the same, and so do the `pr review` description and the help for `--approve`, `--comment` and `--request-changes`. Behaviour is unchanged. ## Flags and settings None ## Measurements main: d40506c5 (2026-11-13) this PR: docs/review-verdict @ c57cbf58 Hardware: Linux (CachyOS), Python 3; no GPU involved Scene: none: docs and a command's help text, nothing in the game Method: `tests/test_forge_pr.py`; `python -I scripts/forge.py pr review --help`; `python -I -B tests/run_all.py` on main and on this PR How to reproduce: `python -I scripts/forge.py pr review --help` | | main @ d40506c5 | this PR @ c57cbf58 | |---|---|---| | `pr review --help` names when to request changes | no | yes | | `tests/test_forge_pr.py` | 25 pass | 25 pass | | `tests/run_all.py` | 461 passed, 7 failed, 69 skipped | 461 passed, 7 failed, 69 skipped | ``` $ python -I -B tests/test_forge_pr.py Ran 25 tests in 0.158s OK $ python -I -B tests/run_all.py 461 passed, 7 failed, 69 skipped in 60 s (linux, 24 at once) ``` ## Testing **Tested:** - `tests/test_forge_pr.py` on Linux: all 25 pass. - `pr review --help` shows the new help text. - `python -I -B tests/run_all.py` on Linux: 461 passed, 7 failed, 69 skipped. The same 7 fail on `main` on this PC, which comes from this PC's environment: `test_bake_portability`, `test_builder_log_read_only`, `test_program_cache_cleanup`, `test_control`, `test_longshot_ctl`, `test_program_cache_one_variant`, `test_vram_budget_row`. **Not tested:** - Nothing in the game: the change is docs and help text only. **Needs other testers:** None ## Screenshots Nothing on screen ## Risk and rollback None at runtime: only text changes. Reverting the commit is enough. ## Reviewer notes The CLI doesn't enforce the rule, because whether a change is blocking is the reviewer's call. ## Issues None ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [x] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] Docs updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

HumanBean force-pushed docs/review-verdict from 2673fc00b7

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been skipped

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been skipped

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been skipped

Details

Third-party parts / The same as upstream (pull_request) Has been skipped

Details

PR description / Description follows the template (pull_request_target) Successful in 7s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

to c57cbf58b7

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been skipped

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been skipped

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been skipped

Details

Third-party parts / The same as upstream (pull_request) Has been skipped

Details

CI / Conventional Commit title (pull_request) Successful in 6s

Details

CI / Workflow lint (actionlint) (pull_request) Failing after 9s

Details

Third-party parts / As their notes say (pull_request) Successful in 13s

Details

PR description / Description follows the template (pull_request_target) Successful in 7s

Details

2026-11-13 06:08:15 +00:00

Compare

HumanBean commented

2026-11-13 06:09:20 +00:00

Author

Owner

Rebased onto d40506c5 (#133, the issue labels). It didn't conflict. test_forge_pr and run_all were run again, with the same results.

Rebased onto d40506c5 (#133, the issue labels). It didn't conflict. test_forge_pr and run_all were run again, with the same results.

HumanBean changed title from ~~WIP: docs(forge): a review that asks for a blocking change requests changes~~ to docs(forge): a review that asks for a blocking change requests changes

2026-11-13 06:09:21 +00:00

HumanBean merged commit a7aa850b17 into main

2026-11-13 06:13:08 +00:00

HumanBean deleted branch docs/review-verdict

2026-11-13 06:13:09 +00:00

HumanBean referenced this pull request from a commit

2026-11-13 06:13:10 +00:00

docs(forge): a review that asks for a blocking change requests changes (#135)