feat(forge): pr comment --attach puts pictures in the comment #254
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!254
Loading…
Reference in a new issue
No description provided.
Delete branch "tonesi/Longshot:feat/forge-comment-attach"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
Test results need screenshots (the testing skill asks for them in its report), but
forge.py pr commentcan post textonly, so every tester puts their pictures up some other way. With
--attach, the pictures go into the comment from thesame command.
What changed
pr comment N [TEXT | --body TEXT | --body-file F] [--attach FILE ...]inscripts/forge.py.post_with_pictures()posts the comment, puts each file up as an attachment of it (upload_picture()andapi_upload():POST /repos/{owner}/{repo}/issues/comments/{id}/assets, multipart fieldattachment, the realname in
?name=), then edits the comment (PATCH /repos/{owner}/{repo}/issues/comments/{id}) so the pictures show.attach:NAME(; NAME as given orpercent-encoded) becomes that file's
browser_download_url; a file the text doesn't show goes at the end as(placeholder_body(),show_pictures()). Anattach:naming no--attachfile is refused.check_picture()andread_pictures(): only PNG, JPEG, GIF and WebP, by extension and by magic bytes (theymust agree), 25 MiB at most each (
ATTACH_MAX), all checked before anything is posted. A refusal lists every bad fileand points to CONTRIBUTING.md's rule that no game files or anything made from them leave the PC (screenshots are
fine). Each file is read once, so what is checked is what goes up.
which pictures went up, which failed (with the server's answer) and which weren't tried. The comment is edited to show
the ones that went up, and the rest keep their
attach:NAME. Without--attach,pr commentruns exactly the line itran before.
tests/test_forge_pr.py: the stand-in Forgejo gets comment ids, the comment edit and the attachment upload.Six new tests: no
--attachposts the text as before (anattach:in it untouched); placeholders replaced and unshownpictures appended, in the order post, uploads, edit; odd names (UTF-8, brackets, spaces) in
?name=, in theplaceholder and percent-encoded in a reference; refusals before posting (missing file, folder,
.nso,.binwith aPNG's bytes,
.pngwith ELF bytes,.pngwith JPEG bytes, empty file, two files of one name, too big, anattach:with no file); a failed upload (the link, what went up, failed and wasn't tried, no retry, the partial edit; a first
upload failing; the edit failing); and the upload request itself (URL, headers and the multipart form read back with
email.parser, a short answer, HTTP 413, no connection).docs/forge.md: thepr commentusage lines and a short Pictures in a comment section..claude/skills/testing/SKILL.md, section 5: one line saying screenshots go up with--attach.Flags and settings
forge.py pr comment --attach FILE ...Measurements
A change to a script: measured by its game-free checks.
main:
fbd42024(2026-11-13)this PR: feat/forge-comment-attach @
79475c05Hardware: i7-12700K, 15 GB RAM, Pop!_OS 24.04 (Linux 7.2), Python 3.12.3; no GPU involved
Scene: none (game-free: a script and its checks)
Method:
tests/run_all.py --jobs 8through this PC's test gate (idle class, one run at a time, fresh HOME), two runson each side, interleaved (this PR, main, this PR, main); this PR's
tests/test_forge_pr.pyalso run against main'sforge.py. The API calls themselves were used for real before this PR, by a standalone script with the same calls:#95 (comment) has 8 PNG attachments, which Forgejo
accepted, and the edit that shows them.
How to reproduce:
python -I -B tests/run_all.py;python -I -B tests/test_forge_pr.pyfbd4202479475c05tests/run_all.py, median of 2 (both runs alike)tests/test_forge_pr.py(each side's own)tests/test_forge_pr.pyagainst the side'sforge.py--attachon main)The 29 skips are the same on both sides: Windows-only checks, tools this PC lacks (spirv-val, glslang, dav1d), a
Vulkan device, and the game's dump.
scripts/forge.pyandtests/test_forge_pr.pyare the same on main at2e1e2e8cand
fbd42024, so the rows against main'sforge.py(run at2e1e2e8c) hold atfbd42024. Before the last rebase, twopairs at main @
2e1e2e8cand this PR @ 138d8a32 (the same change with a 10 MiB limit) gave 500 passed, 0 failed, 29skipped on each side, and one pair before that (main @
7315957c, this PR @ 9601b665) 499 passed on each side.Testing
Tested: on this PC (Linux), game-free:
tests/test_forge_pr.py: the 25 earlier tests unchanged and the 6 new ones above, all passing;forge.py(a scratch copy where the stand-in'sapi_uploadpatch is made withcreate=True, so the file loads): the 5 attach tests fail and the no---attachtest passes;of unshown pictures, no refusal of an
attach:with no file;forge.py pr comment --help.Not tested:
comment's attachment, edit) posted #95 (comment) from a
standalone script; this PR writes them in
forge.py's own style. The first real use (a test result withscreenshots) should print the comment's link, then "N pictures in the comment", with the pictures visible.
[attachment] ALLOWED_TYPESdecides(Forgejo's default list has all four); a refusal would show as a failed upload with the server's answer.
open()andos.path, andthe names are percent-encoded UTF-8 in
?name=.Needs other testers:
python -I -B tests/test_forge_pr.py, and one realpr comment <n> --body-file F --attach shot.pngfrom a folder with a non-ASCII name, expecting 31 tests OK and the picture in the comment (T-XZWG)
Screenshots
Nothing on screen: a script. What the same API calls produce on a pull request:
#95 (comment)
Risk and rollback
pr commentwith--attachchanges; without it the command runs the same line as before. Reverting the commitremoves it.
attach:NAMEfor the pictures that didn't go up. The error saysso, with the comment's link and what to do (add the rest by hand or in a new comment); there is no retry and no
second post.
That stays the poster's responsibility (CONTRIBUTING.md).
a 4K PNG is about 14 to 24 MiB and fits. Pictures can then stay lossless, which a comment needs when it says two
shots are pixel-identical. Larger ones are refused with "crop it, or save it as JPEG or WebP".
ATTACH_MAXis oneconstant: the server's
[attachment] MAX_SIZEis 2048 MB, and Cloudflare cuts an upload at 100 MB(docs/releases.md).
--attachtakes several files, so a TEXT written after it is read as a file (and refused as missing): TEXT goesbefore
--attach, or in--body(the help and docs/forge.md say so).#120 makes forge.py's output UTF-8; the post, the uploads and
the edit all happen before any file name is printed.
Reviewer notes
post_with_pictures()andcheck_picture()inscripts/forge.py.attach:NAMEplaceholders and edited once the pictures are up (theattachments belong to the comment, so it has to exist first; a notification e-mail carries that first version). An
upload that fails stops the rest rather than trying each, since a failure is most likely the connection or the
sign-in. The form's file name is ASCII only; the real name goes in
?name=, which Forgejo takes first.issue comment --attach(the same functions would serve it) and pictures inpr update --comment.main()andtests/test_forge_pr.pytoo, notthe same lines: whichever merges second rebases.
Issues
None.
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened.🤖 Generated with Claude Code
pr comment could post text only, but test results ask for screenshots (.claude/skills/testing/SKILL.md, section 5), so every tester worked around it. --attach FILE ... posts the comment, uploads each file as an attachment of it (POST issues/comments/{id}/assets) and edits the comment so the pictures show: attach:NAME in the text becomes the file's link, and a file the text doesn't show goes at the end. Only PNG, JPEG, GIF and WebP of 25 MiB at most, by extension and by first bytes, checked before anything is posted (no game files leave the PC: CONTRIBUTING.md), as is an attach: with no file. A failed upload stops there, with no retry, and names the comment, what went up and what didn't. Without --attach, pr comment is unchanged. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <[email protected]>Rebased onto main
fbd42024and raised the picture limit from 10 to 25 MiB, so a 4K PNG goes up as it is (lossless, for pixel comparisons). Measured again at79475c05against mainfbd42024: two interleaved pairs, 500 passed, 0 failed, 29 skipped on each side. Windows test request: T-XZWG.WIP: feat(forge): pr comment --attach puts pictures in the commentto feat(forge): pr comment --attach puts pictures in the commentApprove. I read the code and did not run it. This is a careful change, and it can merge as it is.
What's good:
POST /repos/{owner}/{repo}/issues/comments/{id}/assets, fieldattachment). The real name goes in?name=, which Forgejo takes before the form's file name, so the ASCII-only form filename is a sensible way to keep the multipart header simple. The multipart body is built with the stdlib only, and its boundary is random.api_upload()gets its token throughtoken()→auth.forge_session(), likeapi()andapi_text(). A renewal therefore goes through the session lock and saves the new refresh token. The upload adds no second refresh path..binwith PNG bytes or a.pngwith ELF bytes is refused. Each file is read once in binary and capped at 25 MiB, and the bytes that were checked are the bytes that go up. Every problem is refused before anything is posted, and the refusal points to the no-game-files rule. The module docstring,--help, docs/forge.md and the testing skill all say that screenshots are fine and that game files are not.urlopen, and the realapi_upload()form is parsed back withemail.parser. main has not changedforge.py,docs/forge.mdor the test since the fork point (fbd42024), so the "Rebased" box holds.Should change (nit):
). A file whose name has a parenthesis, such asshot (2).png, must be written percent-encoded, just like a name with a space. If it isn't, it is refused before posting, so nothing goes wrong, but the docs mention only spaces. See the inline comment.@ -133,0 +134,4 @@## Pictures in a commentA test result or a review often needs screenshots: `pr comment --attach` puts them in the comment. In the text,`` shows the file `main.png` there (a name with a space is written percent-encoded,Nit:
ATTACH_REFstops at)as well as at whitespace, so a name with a parenthesis (shot (2).png) also has to be written percent-encoded (attach:shot%20%282%29.png). If it isn't, it's refused before posting, so nothing is lost. Maybe say "a name with a space or a parenthesis" here and in the docstring.