MagicBeanDev/Longshot

Watch

Fork 98

fix(forge): show formal reviews and inline feedback in view #310

Open

monsterlag wants to merge 3 commits from monsterlag/Longshot:fix/forge-review-feedback into main

monsterlag commented

2026-11-13 21:17:51 +00:00

First-time contributor

Why

Contributors following review feedback with forge.py pr view --comments cannot see formal review summaries or inline comments. A requested-changes verdict can appear in status while its actual explanation is missing from the documented reading command.

What changed

scripts/forge.py: include submitted review verdicts, summaries and inline feedback with the existing optional discussion output. Print the review commit and thread links, mark dismissed/stale reviews and resolved threads, and preserve the API's supplied file positions.
Page all three feeds, deduplicate by the appropriate review/comment identifiers and URLs, and stop when a page adds no new IDs. The last condition matters on the current server: its issue-comment and inline endpoints repeat the same entries when page changes. Drafts and unanswered review requests are omitted.
tests/test_forge_pr.py: eight CLI regression methods cover the output, overlapping APIs and ID namespaces, state filtering, original/resolved locations, server caps of 20/50, ignored page parameters, unchanged default output and explicit API failure. Existing network guards remain active.
Four docs/tested/ files record the current CPU-only regression runs.
docs/forge.md: document the added feedback and pagination behavior. Issue view and review posting are unchanged.

Flags and settings

None. The existing --comments option includes the additional feedback; default view output is unchanged.

Measurements

main: 9c9d69ed09 (2026-11-14)
this PR: fix/forge-review-feedback @ 4aa011a1e6
Hardware: Ryzen 7 9800X3D, 96 GiB RAM, Windows 11 build 26300, native Python 3.14.7. No GPU used by this check.
Scene: authored, network-guarded CLI cases and live metadata from PR277. No game scene.
Method: focused check as warm-up; two sequential interleaved main/PR runs of the same eight new regression methods, then two interleaved live CLI pairs. Live calls now use ordinary Windows piped output without -X utf8: main includes the separate stream fix from PR120. No performance improvement is claimed.
How to reproduce: run the eight PR.test_review_view_* methods in tests/test_forge_pr.py; for main, load main's scripts/forge.py with the same new tests. Run python -I -B scripts/forge.py pr view 277 --comments on both checkouts. All live calls are read-only API metadata requests; displayed HTML/attachment URLs are never opened.

Check	main @ `9c9d69ed`	this PR @ `4aa011a1`
Regression methods run, each run	8	8
Assertion/subtest failures, run 1 / run 2	9 / 9	0 / 0
Median assertion/subtest failures	9	0
Live formal review summaries, run 1 / run 2	0 / 0	2 / 2
Median formal review summaries	0	2
Live inline comments, run 1 / run 2	0 / 0	2 / 2
Median inline comments	0	2

main, each regression run: methods 8, failures 9, errors 0, skipped 0
this PR, each regression run: methods 8, failures 0, errors 0, skipped 0
main live CLI, each run: exit 0, review summaries 0, inline comments 0
this PR live CLI, each run: exit 0, review summaries 2, inline comments 2

--- review by monsterlag, 2026-11-13 19:18 (request changes, dismissed, stale), commit 1fce28d5
--- inline scripts/oot_saves.py:439
--- inline scripts/oot_saves.py:444
--- review by monsterlag, 2026-11-13 20:47 (approved), commit 6ea909e4

The live summaries/positions were observed on 2026-11-14. The failing-assertion count includes pagination subtests, so it can exceed the number of methods.

Testing

Tested: native Windows focused CLI check; two fresh interleaved eight-method regression pairs on main9c9d69ed and the carried PR (9 assertion/subtest failures on main, zero here, errors/skips0); two live metadata-only PR277 pairs (main0/0 formal/inline, here2/2, all exits0); independent read-only review. Full python -I -B tests/run_all.py --jobs 8 on d00ff970: 533 passed, 3 failed, 34 skipped in 257s. The same three groups fail on untouched main9c9d69ed in fresh focused checks: test_oot_state_tap.py (Windows bridge connection race, issue481), test_tested_configurations.py (authored stream newline assumption, issue482), and test_walk_key.py (missing stand-in after the Skip-title merge, already claimed by mooki). No assertion was weakened and no skip was added.

The final head 4aa011a1 adds only the four measured-run records after that suite; production/test files match the measured tree. The repository-fragment validation passes after those records, and git diff --check passes. History was preserved by merging the frozen main rather than rewriting published commits.

Not tested: live CLI on Linux/macOS, unpublished drafts on the server, concurrent API mutations or every Forgejo version. State/overlap fixtures remain guarded; live verification used submitted reviews. File positions are supplied by the API, not an inferred diff-side mapping. This change does not run in the game.

Needs other testers: None for this read-only CLI fix.

Screenshots

CLI output is shown in Measurements. Nothing in the game changes.

Risk and rollback

--comments makes additional read-only metadata requests and shows grouped review feedback after ordinary discussion. An API error is reported as a failed command rather than silently omitting reviews. Reverting this commit restores the old display. No review posting, authentication, game data or attachment handling changes.

Reviewer notes

Start at api_pages and show_pr_comments. Review IDs are distinct from issue-comment IDs; only inline IDs and supplied URLs suppress overlapping discussion entries. The pagination fixture exercises both real-world server behaviors: clamped page size and ignored page number. Main now includes PR120's output-stream setup; this change remains separate from the attachment listings in PR298/300.

Issues

Fixes #301

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

A file in docs/tested/ for each new measured run (format in docs/tested-configurations.md; no appended row or assembled view).
AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
A game-free test added for the fix, failing on main and passing here.
A check under tests/ that reads changed source updated in this PR, not weakened.
Docs updated.

## Why Contributors following review feedback with `forge.py pr view --comments` cannot see formal review summaries or inline comments. A requested-changes verdict can appear in status while its actual explanation is missing from the documented reading command. ## What changed - `scripts/forge.py`: include submitted review verdicts, summaries and inline feedback with the existing optional discussion output. Print the review commit and thread links, mark dismissed/stale reviews and resolved threads, and preserve the API's supplied file positions. - Page all three feeds, deduplicate by the appropriate review/comment identifiers and URLs, and stop when a page adds no new IDs. The last condition matters on the current server: its issue-comment and inline endpoints repeat the same entries when page changes. Drafts and unanswered review requests are omitted. - `tests/test_forge_pr.py`: eight CLI regression methods cover the output, overlapping APIs and ID namespaces, state filtering, original/resolved locations, server caps of 20/50, ignored page parameters, unchanged default output and explicit API failure. Existing network guards remain active. - Four `docs/tested/` files record the current CPU-only regression runs. - `docs/forge.md`: document the added feedback and pagination behavior. Issue view and review posting are unchanged. ## Flags and settings None. The existing `--comments` option includes the additional feedback; default view output is unchanged. ## Measurements main: 9c9d69ed09e2c24965d35f1696aa5c8dd8fe6e40 (2026-11-14) this PR: fix/forge-review-feedback @ 4aa011a1e6469dcd36cfb46641df79a5071ad28e Hardware: Ryzen 7 9800X3D, 96 GiB RAM, Windows 11 build 26300, native Python 3.14.7. No GPU used by this check. Scene: authored, network-guarded CLI cases and live metadata from PR277. No game scene. Method: focused check as warm-up; two sequential interleaved main/PR runs of the same eight new regression methods, then two interleaved live CLI pairs. Live calls now use ordinary Windows piped output without `-X utf8`: main includes the separate stream fix from PR120. No performance improvement is claimed. How to reproduce: run the eight `PR.test_review_view_*` methods in `tests/test_forge_pr.py`; for main, load main's `scripts/forge.py` with the same new tests. Run `python -I -B scripts/forge.py pr view 277 --comments` on both checkouts. All live calls are read-only API metadata requests; displayed HTML/attachment URLs are never opened. | Check | main @ 9c9d69ed | this PR @ 4aa011a1 | |---|---|---| | Regression methods run, each run | 8 | 8 | | Assertion/subtest failures, run 1 / run 2 | 9 / 9 | 0 / 0 | | Median assertion/subtest failures | 9 | 0 | | Live formal review summaries, run 1 / run 2 | 0 / 0 | 2 / 2 | | Median formal review summaries | 0 | 2 | | Live inline comments, run 1 / run 2 | 0 / 0 | 2 / 2 | | Median inline comments | 0 | 2 | ```text main, each regression run: methods 8, failures 9, errors 0, skipped 0 this PR, each regression run: methods 8, failures 0, errors 0, skipped 0 main live CLI, each run: exit 0, review summaries 0, inline comments 0 this PR live CLI, each run: exit 0, review summaries 2, inline comments 2 --- review by monsterlag, 2026-11-13 19:18 (request changes, dismissed, stale), commit 1fce28d5 --- inline scripts/oot_saves.py:439 --- inline scripts/oot_saves.py:444 --- review by monsterlag, 2026-11-13 20:47 (approved), commit 6ea909e4 ``` The live summaries/positions were observed on 2026-11-14. The failing-assertion count includes pagination subtests, so it can exceed the number of methods. ## Testing **Tested:** native Windows focused CLI check; two fresh interleaved eight-method regression pairs on main9c9d69ed and the carried PR (9 assertion/subtest failures on main, zero here, errors/skips0); two live metadata-only PR277 pairs (main0/0 formal/inline, here2/2, all exits0); independent read-only review. Full `python -I -B tests/run_all.py --jobs 8` on d00ff970: **533 passed, 3 failed, 34 skipped in 257s**. The same three groups fail on untouched main9c9d69ed in fresh focused checks: `test_oot_state_tap.py` (Windows bridge connection race, issue481), `test_tested_configurations.py` (authored stream newline assumption, issue482), and `test_walk_key.py` (missing stand-in after the Skip-title merge, already claimed by mooki). No assertion was weakened and no skip was added. The final head 4aa011a1 adds only the four measured-run records after that suite; production/test files match the measured tree. The repository-fragment validation passes after those records, and `git diff --check` passes. History was preserved by merging the frozen main rather than rewriting published commits. **Not tested:** live CLI on Linux/macOS, unpublished drafts on the server, concurrent API mutations or every Forgejo version. State/overlap fixtures remain guarded; live verification used submitted reviews. File positions are supplied by the API, not an inferred diff-side mapping. This change does not run in the game. **Needs other testers:** None for this read-only CLI fix. ## Screenshots CLI output is shown in Measurements. Nothing in the game changes. ## Risk and rollback `--comments` makes additional read-only metadata requests and shows grouped review feedback after ordinary discussion. An API error is reported as a failed command rather than silently omitting reviews. Reverting this commit restores the old display. No review posting, authentication, game data or attachment handling changes. ## Reviewer notes Start at `api_pages` and `show_pr_comments`. Review IDs are distinct from issue-comment IDs; only inline IDs and supplied URLs suppress overlapping discussion entries. The pagination fixture exercises both real-world server behaviors: clamped page size and ignored page number. Main now includes PR120's output-stream setup; this change remains separate from the attachment listings in PR298/300. ## Issues Fixes #301 ## 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] A file in `docs/tested/` for each new measured run (format in `docs/tested-configurations.md`; no appended row or assembled view). - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Docs updated.

monsterlag added 1 commit

2026-11-13 21:17:51 +00:00

fix(forge): show formal reviews and inline feedback in view

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

Details

Website / Website on Cloudflare (pull_request_target) Has been skipped

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

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

Details

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

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (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

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

Details

afc74a1f89

Read submitted review feedback alongside optional PR discussion, keeping
review and comment IDs separate and default view output unchanged.
Stop pagination on repeated IDs as well as empty responses because some
current server endpoints ignore the page argument.

Add guarded CLI regressions for review state, overlap, locations, capped
and ignored pagination, default output and API failures. Document it.

Assisted-by: OpenAI Codex
Co-Authored-By: OpenAI Codex <[email protected]>

monsterlag changed title from ~~WIP: fix(forge): show formal reviews and inline feedback in view~~ to fix(forge): show formal reviews and inline feedback in view

2026-11-13 21:18:23 +00:00

mooki added the

kind

bug

label

2026-11-14 01:46:07 +00:00

mooki added the

priority

high

label

2026-11-14 04:18:35 +00:00

APusija04 referenced this pull request

2026-11-14 05:05:21 +00:00

[RX 6600] Game crashes with GPU timeout #365

monsterlag added 2 commits

2026-11-14 16:29:40 +00:00

Merge current main into fix/forge-review-feedback d00ff970e2

Carry the focused review-reader change onto 9c9d69ed and include the
separate UTF-8 output-stream fix merged upstream. Preserve published
history and refresh regression/live evidence on the resulting tree.

Assisted-by: OpenAI Codex
Co-Authored-By: OpenAI Codex <[email protected]>

docs(tested): record current game-free regression comparisons

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

Website / Website on Cloudflare (pull_request) Blocked by required conditions

Details

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

Details

Website / Website on Cloudflare (pull_request_target) Has been skipped

Details

4aa011a1e6

Keep each measured native regression run self-contained under docs/tested
using the current contribution format. These are CPU-only tool checks;
they make no gameplay, GPU compatibility or performance claim.

Assisted-by: OpenAI Codex
Co-Authored-By: OpenAI Codex <[email protected]>

monsterlag commented

2026-11-14 16:31:09 +00:00

Author

First-time contributor

Merged main9c9d69ed without rewriting published history and refreshed two native regression/live pairs. Full suite533pass/3main-baseline-fail/34skip; the three Windows check failures are named with fresh main reproductions. Final record-only commit adds four CPU-only run files; fragment validation passes. Live output now works without the old UTF8 override because PR120 is on main.

mooki added the

platform/linux

platform/windows

labels

2026-11-14 18:22:58 +00:00