feat(forge): pr review posts a review with inline comments

HumanBean commented

2026-11-13 05:23:26 +00:00

Owner

Why

Reviews on git.magicbean.dev had no command: posting one with comments on lines meant hand-writing calls to Forgejo's API, so agents and people tended to put everything in one plain comment. forge.py pr review posts a real review (approve, comment or request changes) with its comments beside the lines they're about.

What changed

forge: cmd_pr_review() in scripts/forge.py, pr review N (--approve | --comment | --request-changes) [--body TEXT | --body-file F] [--inline F.json] [--at PATH:LINE TEXT].... It posts one review to POST /repos/MagicBeanDev/Longshot/pulls/N/reviews, tied to the PR's head commit (commit_id), so a later push marks its comments outdated.
- --at PATH:LINE TEXT (repeatable) comments on line LINE of the PR's version of the file; PATH:-LINE on a line the PR removes. --inline F takes a JSON list of {"path", "line" | "old_line", "body"}. Both can be used together.
- diff_lines() reads the PR's .diff (hunks, context included; new, deleted and renamed files), and check_inline() refuses a line outside the hunks before anything is posted, since Forgejo would put that comment on no line.
- It refuses an approval of a work-in-progress PR (AGENTS.md: a WIP PR isn't measured yet), a review of a closed or merged PR, and a comment or request-changes review with neither a summary nor inline comments.
docs: a Reviewing a pull request section in docs/forge.md, and the command in its list and in forge.py's own help.
tests: tests/test_forge_pr.py gets the stand-in Forgejo's review POST and four tests: the diff parser (including --- /+++ lines inside a hunk, a new file and a deleted one), an approval with four inline comments from both options, lines off the diff and malformed input refused with nothing posted, and the WIP/empty/closed cases.

Flags and settings

None.

Measurements

main: 316f9319 (2026-11-13)
this PR: feat/pr-review @ 871ffc34
Hardware: Linux (CachyOS), Python 3; no GPU involved
Scene: none: a command-line tool, nothing in the game
Method: tests/test_forge_pr.py (offline, against the stand-in Forgejo); diff_lines() run on the real diffs of #107 and #108 against the lines the reviews posted there by hand landed on; python -I -B tests/run_all.py
How to reproduce: python -I -B tests/test_forge_pr.py; python -I scripts/forge.py pr review --help

	main @ `316f9319`	this PR @ `871ffc34`
`forge.py pr review`	no such command	posts a review with inline comments
`tests/test_forge_pr.py`	20 tests pass	24 tests pass
`diff_lines()` on #108: `gpu/vk/vk_temporal.cpp:1011`, `tests/test_psychov35_dlss.py:131` (where review 1083 landed)		both in the hunks
`diff_lines()` on #107: `texture_pass.cpp:450`, `vk_draw.cpp:3598` (review 1082); `vk_draw.cpp:3000` (not in the diff)		in, in, refused

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

Testing

Tested:

tests/test_forge_pr.py on Linux: 24 of 24 pass.
The diff parser on the real diffs of #107 and #108: every line the hand-posted reviews there commented on is accepted, and a line outside the diff is refused.
python -I -B tests/run_all.py on Linux: 460 passed, 7 failed, 69 skipped. The 7 fail on main on this PC too (its 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:

A live post through this command: the payload is the one that posted reviews 1082 (#107) and 1083 (#108) by hand through forge.api(), with old_position and commit_id added; the first real review with it will show it.
Windows: the code is standard library Python and reads files as UTF-8, like the other commands.

Needs other testers:
None

Screenshots

Nothing on screen.

Risk and rollback

Only pr review is new; the other commands are untouched. Reverting the commit removes it.

Reviewer notes

Start at cmd_pr_review() and diff_lines() in scripts/forge.py. Not done on purpose: showing reviews' inline comments in pr view --comments (a follow-up), and a pending (draft) review to add to later.

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.
A check under tests/ that reads changed source updated in this PR, not weakened.
Docs updated.

🤖 Generated with Claude Code

## Why Reviews on git.magicbean.dev had no command: posting one with comments on lines meant hand-writing calls to Forgejo's API, so agents and people tended to put everything in one plain comment. `forge.py pr review` posts a real review (approve, comment or request changes) with its comments beside the lines they're about. ## What changed - forge: `cmd_pr_review()` in `scripts/forge.py`, `pr review N (--approve | --comment | --request-changes) [--body TEXT | --body-file F] [--inline F.json] [--at PATH:LINE TEXT]...`. It posts one review to `POST /repos/MagicBeanDev/Longshot/pulls/N/reviews`, tied to the PR's head commit (`commit_id`), so a later push marks its comments outdated. - `--at PATH:LINE TEXT` (repeatable) comments on line LINE of the PR's version of the file; `PATH:-LINE` on a line the PR removes. `--inline F` takes a JSON list of `{"path", "line" | "old_line", "body"}`. Both can be used together. - `diff_lines()` reads the PR's `.diff` (hunks, context included; new, deleted and renamed files), and `check_inline()` refuses a line outside the hunks before anything is posted, since Forgejo would put that comment on no line. - It refuses an approval of a work-in-progress PR (AGENTS.md: a WIP PR isn't measured yet), a review of a closed or merged PR, and a comment or request-changes review with neither a summary nor inline comments. - docs: a *Reviewing a pull request* section in `docs/forge.md`, and the command in its list and in `forge.py`'s own help. - tests: `tests/test_forge_pr.py` gets the stand-in Forgejo's review POST and four tests: the diff parser (including `--- `/`+++ ` lines inside a hunk, a new file and a deleted one), an approval with four inline comments from both options, lines off the diff and malformed input refused with nothing posted, and the WIP/empty/closed cases. ## Flags and settings None. ## Measurements main: 316f9319 (2026-11-13) this PR: feat/pr-review @ 871ffc34 Hardware: Linux (CachyOS), Python 3; no GPU involved Scene: none: a command-line tool, nothing in the game Method: `tests/test_forge_pr.py` (offline, against the stand-in Forgejo); `diff_lines()` run on the real diffs of #107 and #108 against the lines the reviews posted there by hand landed on; `python -I -B tests/run_all.py` How to reproduce: `python -I -B tests/test_forge_pr.py`; `python -I scripts/forge.py pr review --help` | | main @ 316f9319 | this PR @ 871ffc34 | |---|---|---| | `forge.py pr review` | no such command | posts a review with inline comments | | `tests/test_forge_pr.py` | 20 tests pass | 24 tests pass | | `diff_lines()` on #108: `gpu/vk/vk_temporal.cpp:1011`, `tests/test_psychov35_dlss.py:131` (where review 1083 landed) | | both in the hunks | | `diff_lines()` on #107: `texture_pass.cpp:450`, `vk_draw.cpp:3598` (review 1082); `vk_draw.cpp:3000` (not in the diff) | | in, in, refused | ``` $ python -I -B tests/test_forge_pr.py Ran 24 tests in 0.140s OK $ python -I -B tests/run_all.py 460 passed, 7 failed, 69 skipped in 60 s (linux, 24 at once) ``` ## Testing **Tested:** - `tests/test_forge_pr.py` on Linux: 24 of 24 pass. - The diff parser on the real diffs of #107 and #108: every line the hand-posted reviews there commented on is accepted, and a line outside the diff is refused. - `python -I -B tests/run_all.py` on Linux: 460 passed, 7 failed, 69 skipped. The 7 fail on `main` on this PC too (its 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:** - A live post through this command: the payload is the one that posted reviews 1082 (#107) and 1083 (#108) by hand through `forge.api()`, with `old_position` and `commit_id` added; the first real review with it will show it. - Windows: the code is standard library Python and reads files as UTF-8, like the other commands. **Needs other testers:** None ## Screenshots Nothing on screen. ## Risk and rollback Only `pr review` is new; the other commands are untouched. Reverting the commit removes it. ## Reviewer notes Start at `cmd_pr_review()` and `diff_lines()` in `scripts/forge.py`. Not done on purpose: showing reviews' inline comments in `pr view --comments` (a follow-up), and a pending (draft) review to add to later. ## 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] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Docs updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

HumanBean added 1 commit

2026-11-13 05:23:26 +00:00

feat(forge): pr review posts a review with inline comments

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 3s

Details

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

Details

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

Details

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

Details

871ffc34a0

forge.py pr review N (--approve | --comment | --request-changes) posts one
Forgejo review: the verdict, a summary (--body / --body-file) and comments on
lines of the diff, from --at PATH:LINE TEXT (PATH:-LINE for a removed line) or
--inline F (a JSON list). Lines outside the diff's hunks are refused before
anything is posted, the review is tied to the PR's head commit, and a
work-in-progress PR can be commented on but not approved. docs/forge.md has a
Reviewing section; tests/test_forge_pr.py covers it offline.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

HumanBean commented

2026-11-13 05:30:10 +00:00

Author

Owner

Description: Needs other testers is a plain None (the check didn't read 'None: ...' as none), and the If applicable box for the check under tests/ that reads forge.py (test_forge_pr.py) is in. No rebase needed: the branch is on main's head, 316f9319; Forgejo shows a WIP PR as not mergeable.

Description: Needs other testers is a plain None (the check didn't read 'None: ...' as none), and the If applicable box for the check under tests/ that reads forge.py (test_forge_pr.py) is in. No rebase needed: the branch is on main's head, 316f9319; Forgejo shows a WIP PR as not mergeable.

HumanBean changed title from ~~WIP: feat(forge): pr review posts a review with inline comments~~ to feat(forge): pr review posts a review with inline comments

2026-11-13 05:30:11 +00:00

HumanBean referenced this pull request

2026-11-13 05:33:21 +00:00

docs(agents): take a pull request out of WIP yourself once it is ready #126

HumanBean referenced this pull request from a commit

2026-11-13 05:33:40 +00:00

docs(agents): take a pull request out of WIP yourself once it is ready (#126)

HumanBean merged commit ccf590673d into main

2026-11-13 05:33:50 +00:00

HumanBean deleted branch feat/pr-review

2026-11-13 05:33:50 +00:00

HumanBean referenced this pull request from a commit