MagicBeanDev/Hookshot

Archived

Watch

Fork 0

feat(agents): pin test requests and results to commits #2830

Open

github-sync wants to merge 7 commits from github/pr/2830 into main

github-sync commented

2026-11-12 15:49:14 +00:00

Owner

@mooki000 opened this pull request on GitHub · 2026-11-12 15:37 UTC · original

What this is for: testers and authors should be able to tell which commit a test request asks for, and whether a result covers the current pull request. This extends the queue from #2734 and #2746 without replacing its room-message format. What is measured: offline queue tests on Linux, including the old client reading new messages: 225 checks passed. The complete game-free rerun had 474 passed, 1 failed, 20 skipped; the failure is the unchanged release-script concurrency case described below. Three central tests fail against main for the missing commit tracking. The project's whole check set, both windows-2022 parts included, passed on a run on my fork before this was opened. Not measured: a live claim and result (the trial card waits for a maintainer's label), or how the Discord relay treats the added lines beyond what the trial below shows. No game or GPU is needed for this change.

Refs #2797.

Problem

A request names a PR, but a PR can change while someone is building it.

On November 12, #2731's defect report named bce3d17e. The author fixed it in 309e6c35 at 13:52 UTC. A test result posted at 14:43 UTC correctly named its build of bce3d17e, but the queue had no way to distinguish that result from a test of the new head. The test remains useful evidence for its build; it is not evidence for the later fix.

In scripts/test_requests.py, cmd_request recorded the PR number, and replay recorded each result and comment link, but neither stored a commit. The existing GitHub approval response already contains the head SHA.

What changed

Requests pin a commit. request reuses its one GitHub response to store the head. request --commit <sha> deliberately chooses another commit of that PR. Values must be 7–40 hexadecimal characters. A head abbreviation expands to the full head; another value is kept and called out as an explicit non-head choice. The script checks the spelling, not membership in the PR or abbreviation uniqueness. If GitHub cannot be reached, an explicit full 40-character SHA still posts, with approval and head unknown; without it, the error explains that remedy.
Claims say what to build. The existing approval check also checks the head. If it moved since the request, the command and rendered card show both heads and ask the requester to confirm. The pin is not silently changed, and the request is not closed.
Results name the tested commit. done --commit <sha> records the actual build. Without it, the default is the commit recorded for that tester at claim time, even if the requester updated the card during the test. A result differing from the head observed at reporting time says “tested an earlier commit”. GitHub being unreachable or rate-limited never blocks recording a finished test: it writes the commit without a head and explains that the comparison was unavailable.
An author can update one card. update T-XXXX --commit <sha> is restricted to the requester and an open card. It keeps the ID, claims, results and room history. Older results stay visible with their commits, but do not satisfy the new pin or block another claim. Existing testers retain the commit they started with.
Listing keeps the approval display. Plain list calls the same annotation path as main, once per distinct open PR. That same answer supplies the head at no extra API cost. Waiting requests still say they need a maintainer and appear last. show checks its one PR, and claim always checks approval again.
The guide and both request/testing instructions say to build the card's commit and name the actual build in done. The comment still starts Test request T-XXXX: pass|fail|mixed, followed by hardware and the commit built.

No new package, shell command or platform-specific code is used by the client. GITHUB_TOKEN, then GH_TOKEN, remain optional. The script does not retrieve a token from gh.

Message compatibility

The original first line stays unchanged. Commit: and Head: lines are appended after all existing lines, including request notes. For example:

🧪 T-7KQ2 done pass: https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2731
Commit: bce3d17e
Head: 309e6c35

Old requests without these fields render exactly as before. The old client treats added request lines as notes and ignores added claim/result lines; its result URL is unchanged. The new client's per-tester commit map is separate from the old numeric claim-expiry map. An old client's result without a commit still appears, but is not credited to a known commit. Old clients ignore update and cannot enforce pins.

The Discord relay is separate software. This repository does not build its embeds, and the relay's unknown-field handling is not documented. Loading the old client in tests proves client compatibility, not server compatibility. hckrman101, as the relay owner, should confirm that appended lines preserve the card, and that its commit display and update handling agree with the client. The guide keeps the old Discord layout and adds a separate CLI example; it does not claim the live relay already shows the new fields.

GitHub calls

For successful commands on open cards, with N distinct PRs in a listing:

Command	Before	After
`request`	1	1, reused for head and approval
`list`	N	N, reused for head and approval
`show`	1	1
`claim`	1	1
`done`	0	1 best-effort attempt
`renew`, `release`, `cancel`	0	0
`update`	unavailable	1

The existing rate-limit warning remains; there are no retries. Reads still show the room record when GitHub is unavailable. done still posts with Commit: but no Head: and a note. request can also post without a successful lookup when --commit is a full 40-character SHA; otherwise it explains that a known full commit or a later retry is required. Such a card's approval stays unknown until the next successful lookup. Claiming still requires approval; the new update command requires a head check.

How to try it

After the PR is pushed and approved for testing:

python -I scripts/test_requests.py request --pr <number> --summary "<what to test>" --needs "<hardware>"
python -I scripts/test_requests.py claim T-XXXX

Build the commit printed by claim, not whichever head a later fetch returns. Report the actual build:

python -I scripts/test_requests.py done T-XXXX --commit <built-sha> --result pass --comment <PR-comment-URL>

The requester can move the card after a fix:

python -I scripts/test_requests.py update T-XXXX --commit <new-sha>

Use show T-XXXX for a fresh single-card view or list to refresh the queue's approval, PR state and heads together.

How it was tested

Arch Linux, kernel 7.2.15-1-omarchy, Intel Core i9-13900K, Python 3.12, November 12. Base 51da0464; head d22aec74. No game, GPU or window. The whole-suite receipt records load average 7.58 → 46.27; this was a correctness run, not a performance measurement. Tests used fake room/session/GitHub responses, and command tests reject unexpected HTTP calls.

Focused tests and compatibility

python -I -B tests/test_test_requests.py --old-source <main-client-copy>:

pass test_request_head
pass test_moved_head
pass test_done_commit
pass test_explicit_commit
pass test_update_commit
pass test_old_card
pass test_list_calls
pass test_done_unreachable
pass test_request_unreachable
pass test_github_head
LIST CALLS: main=1 branch=1 (two cards, one PR)
pass test_old_client
225 checks
all passed

The old-client copy is the actual scripts/test_requests.py from main 51da0464, loaded alongside the new client. It accepts the new request, claim and done, retains the exact result URL, treats the request's extra fields as notes, and ignores the unknown update action. The stored old-format card test compares its complete rendering byte-for-byte.

The remaining cases cover approval and merged-PR refusals, claim renewal/expiry, explicit and default commits, malformed hex/lengths, upper-case and abbreviated commits, a requester update during an active test, old results without commit metadata, no extra request API call, equal list-call counts against the actual main client, optional-token precedence, and GitHub outages/rate limits. Both claimed/default and explicit done commits still post without Head; requests without a known full SHA fail with the remedy, while an explicit full SHA posts and remains approval-unknown until a later lookup succeeds.

The three central tests against main

The same test file with --source <main-client-copy> --test request_head --test moved_head --test done_commit exits 1, with ordinary assertion failures rather than a missing option or import failure:

FAIL: request records the head commit
FAIL test_request_head
FAIL: claim tells the tester the pinned commit, not the new head
FAIL: claim reports both heads and asks the requester to confirm
FAIL test_moved_head
FAIL: done defaults to the commit recorded at claim time
FAIL: done records the current head and calls the result an earlier commit
FAIL test_done_commit
21 checks
15 failure(s)

All three pass in the branch run above.

Whole game-free suite

python -I -B tests/run_all.py --jobs 8, through the local idle/core-dump wrappers, with the project's pinned dependencies:

474 passed, 1 failed, 20 skipped in 185 s (linux, 8 at once)

Exit 1. The queue tests and tracked-Python source compilation check passed. The one failure is tests/test_release_scripts.py::test_nightly_two_runs_at_once_end_at_the_newer_commit, subcase first='A':

gh: Not Found (HTTP 404)
release_files.sh: nightly: SHA256SUMS still differs from the files after 5 uploads

The case name and message match the known release-script race signature. That test and scripts/release_files.sh are unchanged from pinned main; this is not a current-main rerun or a green-suite claim. The failure was retained, not retried away. The pre-review revision had passed 475/0/20 in 158 s; that earlier result is not a substitute for this rerun. The 20 skips are the runner's existing Windows/game/GPU-only declarations; none was added here.

Real-room trial

One real request, made with this branch's client (d22aec74) for this pull request: T-6BH3.

$ python -I scripts/test_requests.py request --pr 2830 --summary "Test requests pin a commit (queue client)" --needs "..." --notes "..."
asked for testing as T-6BH3
commit to build: d22aec74f83702b20ad8346b5879b6a5fd30dacd
note: #2830 is not from a collaborator and has no "testing wanted" label, so nobody can claim it until a maintainer adds the label on GitHub

The card as this branch's client shows it (show T-6BH3):

T-6BH3  OPEN  https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2830  Test requests pin a commit (queue client)
    asked by you on 2026-11-12 15:37Z
    commit: d22aec74
    needs: Anyone using scripts/test_requests.py, Windows or Linux; no game needed
    waiting for a maintainer to add the "testing wanted" label on GitHub: not claimable yet
    Use the client from this PR: claim prints the commit to build; done records the commit tested. What to check is in the description.

The same card as main's client (51da0464) lists it: everything as before, with the two new lines read as notes:

T-6BH3  OPEN  https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2830  Test requests pin a commit (queue client)
    asked by you on 2026-11-12 15:37Z
    needs: Anyone using scripts/test_requests.py, Windows or Linux; no game needed
    waiting for a maintainer to add the "testing wanted" label on GitHub: not claimable yet
    Use the client from this PR: claim prints the commit to build; done records the commit tested. What to check is in the description.
    Commit: d22aec74f83702b20ad8346b5879b6a5fd30dacd
    Head: d22aec74f83702b20ad8346b5879b6a5fd30dacd

The Discord relay accepted the new message. The card appeared in the test-request channel like every other one (title, "Waiting for a maintainer to add the testing wanted label", the Needs line, "Pull request: #2830", T-6BH3). It does not show the commit, as expected: the relay would need its own small change for that.

Not shown yet: a live claim, done and update. The card cannot be claimed until a maintainer adds the label, so those three ran against the test room only. @hckrman101, as the relay's owner: could you confirm the relay is happy with the extra Commit: / Head: lines on claim and result messages and with the new update action before this is relied on? If it is not, the fallback in Risks (the same data in the event's content, the visible text unchanged) is what I would do.

Not tested

Windows beyond the project's own source checks (they run this test file on windows-2022 and passed on the fork run above); the client was not used against the live room from Windows.
A live Matrix request/claim/result, Discord embed rendering, or the relay's labelled-PR producer.
The client's real GitHub HTTP path; its response parsing, token headers and rate-limit behaviour were exercised with fakes.
A game build or game run; no game code changed. No native C/C++ fixture was added or changed.

Risks

The behaviour changes from main are limited to:

New requests, claims and results carry additive commit/head lines and render their commits. Pinning means a request needs a successful head lookup or an explicit full SHA; a pinned result with no recorded claim commit needs --commit. No approval rule is relaxed.
The new requester-only update action moves the pin without deleting history.
done asks GitHub once to compare heads when it can; a failed lookup is only a note and never blocks the result.
Results for another or unknown commit do not count as tests of the pinned commit or block a fresh claim for it.

Plain list keeps main's approval display and API-call count. The relay's owner should confirm the card rendering before rollout.

A strict relay could reject unexpected body lines, or accept them without displaying the commit. If the real-room trial shows that, stop rollout and agree on structured event-content metadata with the relay owner while leaving the visible body byte-identical. The current change implements appended lines, not that contingency.
Old clients continue to work but cannot enforce pins or understand update. A mixed-version queue can therefore still contain results with unknown commits; these are shown as unknown, not silently assigned a SHA.
Explicit non-head SHAs are syntax-checked only. A wrong or ambiguous SHA needs correction by the requester. “Earlier commit” means different from the head observed then; a force-push need not preserve ancestry.
A head can advance after any GitHub response. The recorded SHA states exactly what that command observed; it is not a promise that the branch will stay there.

Overlaps

The overlap check found three open PRs sharing an edited file; their shared hunks were read:

#2807: a real conflict at docs/test-requests.md:25–40 on this branch (main's line 25). It adds the hardware-runner link to “You can take part…”, immediately after this change's new CLI example. Keep both; whichever lands second rebases. Its label-gated automation enforces the exact labelled commit and checks it is still the PR head before running the test: that is the automated workflow that needs an unambiguous requested/tested commit in this queue. The conflicting hunk does not implement the queue fields.
#2732: development-pack advice in an earlier part of AGENTS.md; merges cleanly with the request instructions.
#2561: the opening documentation links and general rules in AGENTS.md; merges cleanly with the request section.

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]

> [@mooki000](https://github.com/mooki000) opened this pull request on GitHub · 2026-11-12 15:37 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2830) **What this is for:** testers and authors should be able to tell which commit a test request asks for, and whether a result covers the current pull request. This extends the queue from #2734 and #2746 without replacing its room-message format. **What is measured:** offline queue tests on Linux, including the old client reading new messages: **225 checks passed**. The complete game-free rerun had **474 passed, 1 failed, 20 skipped**; the failure is the unchanged release-script concurrency case described below. Three central tests fail against main for the missing commit tracking. The project's whole check set, both `windows-2022` parts included, passed on [a run on my fork](https://github.com/mooki000/Hookshot/actions/runs/43245937287) before this was opened. **Not measured:** a live claim and result (the trial card waits for a maintainer's label), or how the Discord relay treats the added lines beyond what the trial below shows. No game or GPU is needed for this change. Refs #2797. ## Problem A request names a PR, but a PR can change while someone is building it. On November 12, [#2731's defect report](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2731) named `bce3d17e`. The author [fixed it in `309e6c35`](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2731) at 13:52 UTC. A [test result posted at 14:43 UTC](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2731) correctly named its build of `bce3d17e`, but the queue had no way to distinguish that result from a test of the new head. The test remains useful evidence for its build; it is not evidence for the later fix. In `scripts/test_requests.py`, `cmd_request` recorded the PR number, and `replay` recorded each result and comment link, but neither stored a commit. The existing GitHub approval response already contains the head SHA. ## What changed - **Requests pin a commit.** `request` reuses its one GitHub response to store the head. `request --commit <sha>` deliberately chooses another commit of that PR. Values must be 7–40 hexadecimal characters. A head abbreviation expands to the full head; another value is kept and called out as an explicit non-head choice. The script checks the spelling, not membership in the PR or abbreviation uniqueness. If GitHub cannot be reached, an explicit full 40-character SHA still posts, with approval and head unknown; without it, the error explains that remedy. - **Claims say what to build.** The existing approval check also checks the head. If it moved since the request, the command and rendered card show both heads and ask the requester to confirm. The pin is not silently changed, and the request is not closed. - **Results name the tested commit.** `done --commit <sha>` records the actual build. Without it, the default is the commit recorded for that tester at claim time, even if the requester updated the card during the test. A result differing from the head observed at reporting time says **“tested an earlier commit”**. GitHub being unreachable or rate-limited never blocks recording a finished test: it writes the commit without a head and explains that the comparison was unavailable. - **An author can update one card.** `update T-XXXX --commit <sha>` is restricted to the requester and an open card. It keeps the ID, claims, results and room history. Older results stay visible with their commits, but do not satisfy the new pin or block another claim. Existing testers retain the commit they started with. - **Listing keeps the approval display.** Plain `list` calls the same annotation path as main, once per distinct open PR. That same answer supplies the head at no extra API cost. Waiting requests still say they need a maintainer and appear last. `show` checks its one PR, and `claim` always checks approval again. - **The guide and both request/testing instructions** say to build the card's commit and name the actual build in `done`. The comment still starts `Test request T-XXXX: pass|fail|mixed`, followed by hardware and the commit built. No new package, shell command or platform-specific code is used by the client. `GITHUB_TOKEN`, then `GH_TOKEN`, remain optional. The script does not retrieve a token from `gh`. ### Message compatibility The original first line stays unchanged. `Commit:` and `Head:` lines are appended **after all existing lines**, including request notes. For example: ```text 🧪 T-7KQ2 done pass: https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2731 Commit: bce3d17e Head: 309e6c35 ``` Old requests without these fields render exactly as before. The old client treats added request lines as notes and ignores added claim/result lines; its result URL is unchanged. The new client's per-tester commit map is separate from the old numeric claim-expiry map. An old client's result without a commit still appears, but is not credited to a known commit. Old clients ignore `update` and cannot enforce pins. **The Discord relay is separate software.** This repository does not build its embeds, and the relay's unknown-field handling is not documented. Loading the old client in tests proves client compatibility, not server compatibility. hckrman101, as the relay owner, should confirm that appended lines preserve the card, and that its commit display and update handling agree with the client. The guide keeps the old Discord layout and adds a separate CLI example; it does not claim the live relay already shows the new fields. ### GitHub calls For successful commands on open cards, with `N` distinct PRs in a listing: | Command | Before | After | |---|---:|---:| | `request` | 1 | 1, reused for head and approval | | `list` | N | N, reused for head and approval | | `show` | 1 | 1 | | `claim` | 1 | 1 | | `done` | 0 | 1 best-effort attempt | | `renew`, `release`, `cancel` | 0 | 0 | | `update` | unavailable | 1 | The existing rate-limit warning remains; there are no retries. Reads still show the room record when GitHub is unavailable. `done` still posts with `Commit:` but no `Head:` and a note. `request` can also post without a successful lookup when `--commit` is a full 40-character SHA; otherwise it explains that a known full commit or a later retry is required. Such a card's approval stays unknown until the next successful lookup. Claiming still requires approval; the new update command requires a head check. ## How to try it After the PR is pushed and approved for testing: ```sh python -I scripts/test_requests.py request --pr <number> --summary "<what to test>" --needs "<hardware>" python -I scripts/test_requests.py claim T-XXXX ``` Build the commit printed by `claim`, not whichever head a later fetch returns. Report the actual build: ```sh python -I scripts/test_requests.py done T-XXXX --commit <built-sha> --result pass --comment <PR-comment-URL> ``` The requester can move the card after a fix: ```sh python -I scripts/test_requests.py update T-XXXX --commit <new-sha> ``` Use `show T-XXXX` for a fresh single-card view or `list` to refresh the queue's approval, PR state and heads together. ## How it was tested Arch Linux, kernel `7.2.15-1-omarchy`, Intel Core i9-13900K, Python 3.12, November 12. Base `51da0464`; head `d22aec74`. No game, GPU or window. The whole-suite receipt records load average 7.58 → 46.27; this was a correctness run, not a performance measurement. Tests used fake room/session/GitHub responses, and command tests reject unexpected HTTP calls. ### Focused tests and compatibility `python -I -B tests/test_test_requests.py --old-source <main-client-copy>`: ```text pass test_request_head pass test_moved_head pass test_done_commit pass test_explicit_commit pass test_update_commit pass test_old_card pass test_list_calls pass test_done_unreachable pass test_request_unreachable pass test_github_head LIST CALLS: main=1 branch=1 (two cards, one PR) pass test_old_client 225 checks all passed ``` The old-client copy is the actual `scripts/test_requests.py` from main `51da0464`, loaded alongside the new client. It accepts the new request, claim and done, retains the exact result URL, treats the request's extra fields as notes, and ignores the unknown update action. The stored old-format card test compares its complete rendering byte-for-byte. The remaining cases cover approval and merged-PR refusals, claim renewal/expiry, explicit and default commits, malformed hex/lengths, upper-case and abbreviated commits, a requester update during an active test, old results without commit metadata, no extra request API call, equal list-call counts against the actual main client, optional-token precedence, and GitHub outages/rate limits. Both claimed/default and explicit done commits still post without Head; requests without a known full SHA fail with the remedy, while an explicit full SHA posts and remains approval-unknown until a later lookup succeeds. ### The three central tests against main The same test file with `--source <main-client-copy> --test request_head --test moved_head --test done_commit` exits 1, with ordinary assertion failures rather than a missing option or import failure: ```text FAIL: request records the head commit FAIL test_request_head FAIL: claim tells the tester the pinned commit, not the new head FAIL: claim reports both heads and asks the requester to confirm FAIL test_moved_head FAIL: done defaults to the commit recorded at claim time FAIL: done records the current head and calls the result an earlier commit FAIL test_done_commit 21 checks 15 failure(s) ``` All three pass in the branch run above. ### Whole game-free suite `python -I -B tests/run_all.py --jobs 8`, through the local idle/core-dump wrappers, with the project's pinned dependencies: ```text 474 passed, 1 failed, 20 skipped in 185 s (linux, 8 at once) ``` Exit 1. The queue tests and tracked-Python source compilation check passed. The one failure is `tests/test_release_scripts.py::test_nightly_two_runs_at_once_end_at_the_newer_commit`, subcase `first='A'`: ```text gh: Not Found (HTTP 404) release_files.sh: nightly: SHA256SUMS still differs from the files after 5 uploads ``` The case name and message match the known release-script race signature. That test and `scripts/release_files.sh` are unchanged from pinned main; this is not a current-main rerun or a green-suite claim. The failure was retained, not retried away. The pre-review revision had passed `475/0/20` in 158 s; that earlier result is not a substitute for this rerun. The 20 skips are the runner's existing Windows/game/GPU-only declarations; none was added here. ## Real-room trial One real request, made with this branch's client (`d22aec74`) for this pull request: **T-6BH3**. ```text $ python -I scripts/test_requests.py request --pr 2830 --summary "Test requests pin a commit (queue client)" --needs "..." --notes "..." asked for testing as T-6BH3 commit to build: d22aec74f83702b20ad8346b5879b6a5fd30dacd note: #2830 is not from a collaborator and has no "testing wanted" label, so nobody can claim it until a maintainer adds the label on GitHub ``` The card as this branch's client shows it (`show T-6BH3`): ```text T-6BH3 OPEN https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2830 Test requests pin a commit (queue client) asked by you on 2026-11-12 15:37Z commit: d22aec74 needs: Anyone using scripts/test_requests.py, Windows or Linux; no game needed waiting for a maintainer to add the "testing wanted" label on GitHub: not claimable yet Use the client from this PR: claim prints the commit to build; done records the commit tested. What to check is in the description. ``` The same card as main's client (`51da0464`) lists it: everything as before, with the two new lines read as notes: ```text T-6BH3 OPEN https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2830 Test requests pin a commit (queue client) asked by you on 2026-11-12 15:37Z needs: Anyone using scripts/test_requests.py, Windows or Linux; no game needed waiting for a maintainer to add the "testing wanted" label on GitHub: not claimable yet Use the client from this PR: claim prints the commit to build; done records the commit tested. What to check is in the description. Commit: d22aec74f83702b20ad8346b5879b6a5fd30dacd Head: d22aec74f83702b20ad8346b5879b6a5fd30dacd ``` **The Discord relay accepted the new message.** The card appeared in the test-request channel like every other one (title, "Waiting for a maintainer to add the testing wanted label", the Needs line, "Pull request: #2830", T-6BH3). It does not show the commit, as expected: the relay would need its own small change for that. **Not shown yet:** a live `claim`, `done` and `update`. The card cannot be claimed until a maintainer adds the label, so those three ran against the test room only. @hckrman101, as the relay's owner: could you confirm the relay is happy with the extra `Commit:` / `Head:` lines on claim and result messages and with the new `update` action before this is relied on? If it is not, the fallback in Risks (the same data in the event's content, the visible text unchanged) is what I would do. ## Not tested - Windows beyond the project's own source checks (they run this test file on `windows-2022` and passed on the fork run above); the client was not used against the live room from Windows. - A live Matrix request/claim/result, Discord embed rendering, or the relay's labelled-PR producer. - The client's real GitHub HTTP path; its response parsing, token headers and rate-limit behaviour were exercised with fakes. - A game build or game run; no game code changed. No native C/C++ fixture was added or changed. ## Risks The behaviour changes from main are limited to: - New requests, claims and results carry additive commit/head lines and render their commits. Pinning means a request needs a successful head lookup or an explicit full SHA; a pinned result with no recorded claim commit needs `--commit`. No approval rule is relaxed. - The new requester-only `update` action moves the pin without deleting history. - `done` asks GitHub once to compare heads when it can; a failed lookup is only a note and never blocks the result. - Results for another or unknown commit do not count as tests of the pinned commit or block a fresh claim for it. Plain `list` keeps main's approval display and API-call count. The relay's owner should confirm the card rendering before rollout. - A strict relay could reject unexpected body lines, or accept them without displaying the commit. If the real-room trial shows that, stop rollout and agree on structured event-content metadata with the relay owner while leaving the visible body byte-identical. The current change implements appended lines, not that contingency. - Old clients continue to work but cannot enforce pins or understand `update`. A mixed-version queue can therefore still contain results with unknown commits; these are shown as unknown, not silently assigned a SHA. - Explicit non-head SHAs are syntax-checked only. A wrong or ambiguous SHA needs correction by the requester. “Earlier commit” means different from the head observed then; a force-push need not preserve ancestry. - A head can advance after any GitHub response. The recorded SHA states exactly what that command observed; it is not a promise that the branch will stay there. ## Overlaps The overlap check found three open PRs sharing an edited file; their shared hunks were read: - **#2807:** a real conflict at `docs/test-requests.md:25–40` on this branch (main's line 25). It adds the hardware-runner link to “You can take part…”, immediately after this change's new CLI example. Keep both; whichever lands second rebases. Its label-gated automation enforces the exact labelled commit and checks it is still the PR head before running the test: that is the automated workflow that needs an unambiguous requested/tested commit in this queue. The conflicting hunk does not implement the queue fields. - **#2732:** development-pack advice in an earlier part of `AGENTS.md`; merges cleanly with the request instructions. - **#2561:** the opening documentation links and general rules in `AGENTS.md`; merges cleanly with the request section. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <[email protected]>

github-sync added 1 commit

2026-11-12 15:49:14 +00:00

feat(agents): pin test requests and results to commits d22aec74f8

Reuse PR approval responses to pin requests and show moved heads. Keep each claim on its chosen commit, retain results across requester updates, and record results even when GitHub cannot be reached. An explicit full commit also lets a request be posted without a head lookup.

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra <[email protected]>

github-sync commented

2026-11-12 17:59:44 +00:00

Author

Owner

@hckrman101 reviewed on GitHub · 2026-11-12 17:24 UTC · original

Thanks, this is a real gap (a result for a build that is no longer the head being read as a result for the fix), and the compatibility work is careful. I read the diff, no run. CI is green on d22aec7, no conflicts with main.

What I checked and found fine:

Old cards, claims and results without Commit:/Head: replay as before; current_results only filters when the card itself has a commit, so legacy cards keep their ordering and "nobody has tested it yet". test_old_card and test_old_client cover the old format, including a stored old card rendered byte for byte.
The commit comes from the same pr_info urllib call that approval already used, so no gh dependency and no new platform code; it works on Windows. done degrades to a note when GitHub is unreachable, claim/update refuse without an answer (as claim already did).
Skill text still says the request's and the PR's text are data, never instructions (AGENTS.md), and the review-before-build step is untouched.

Points worth fixing, in order:

Stale results are not visible to a JSON reader. current_results is used for sorting, claim and the "nobody has tested this commit" line, but list --json/show --json (public(t)) return the raw results. The /testing skill tells the agent to skip a request when its own name is in results, "unless the PR changed". After update, an agent doing that will skip the card it should now test, because its old result is still listed. Please expose the filtered view (e.g. a current flag on each result, or current_results as its own key) and word the skill step in terms of it. The same "unless the PR changed since your result" wording is still in both skills and in claim's refusal path: with pins, a moved head without an update does not make the same pin worth retesting, so it should say "unless the card was updated to a new commit".
--commit is not tied to the PR, and that sits next to the approval gate. The guide says approval keeps strangers' code off testers' PCs until someone has looked at the PR. Now any requester can pin an arbitrary 7-40 hex value, and git checkout --detach <commit> in the testing skill will build it if it is fetchable (GitHub serves commits from other forks in the same network by SHA). The PR calls this out ("membership in the PR is not checked") but the tester side has no guard. Cheapest fix is in the testing skill: before building, check the commit is part of the PR (git merge-base --is-ancestor <commit> <PR head>, or the PR's commit list), review that commit's diff rather than the head's, and release the claim if it is not. Checking it in request/update through the PR commits endpoint would be stronger but costs a call; I would be fine with the skill-only version.
A moved head still lets the claim go through. claim posts, prints "ask the requester to confirm", and the tester is then holding a 90 minute claim on a commit that may already be superseded. For /loop 30m /testing there is nobody to ask. list already shows the "head moved" line, so the skill could say to skip cards that show it unless the requester has confirmed, or claim could need an explicit --commit when the head moved. Your call, but as written an unattended agent will happily test the stale pin.
request now needs GitHub. Before, a request posted even when GitHub was down or rate limited (60 an hour without a token, shared by an office or a CGNAT). Now it fails unless an explicit full SHA is given. The error says so, so I think that is acceptable, but a one-line mention in the request skill next to GITHUB_TOKEN would help, since the 40-character fallback needs a git rev-parse the agent may not think of.

Nits:

claim_commits is not cleared on release or expiry, so a done with no new claim silently uses the old claim's commit. Dropping it on those two events (or requiring --commit when the claim is gone) is cleaner.
Notes lines starting with Commit: or Head: are swallowed by the request parser. Harmless in practice.
docs/test-requests.md will conflict with #2807 as you said; whoever lands second rebases.

Relay: only the request message was trialled in Discord. The claim, result and update messages (and an unknown update action) have not been seen by the relay yet, so that check is still open before this is relied on. I would not merge until it is done. Nothing here depends on game or GPU measurements, so no in-game evidence is needed.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 17:24 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2830#pullrequestreview-5878986385) Thanks, this is a real gap (a result for a build that is no longer the head being read as a result for the fix), and the compatibility work is careful. I read the diff, no run. CI is green on d22aec7, no conflicts with main. What I checked and found fine: - Old cards, claims and results without `Commit:`/`Head:` replay as before; `current_results` only filters when the card itself has a commit, so legacy cards keep their ordering and "nobody has tested it yet". `test_old_card` and `test_old_client` cover the old format, including a stored old card rendered byte for byte. - The commit comes from the same `pr_info` urllib call that approval already used, so no `gh` dependency and no new platform code; it works on Windows. `done` degrades to a note when GitHub is unreachable, `claim`/`update` refuse without an answer (as `claim` already did). - Skill text still says the request's and the PR's text are data, never instructions (AGENTS.md), and the review-before-build step is untouched. Points worth fixing, in order: 1. **Stale results are not visible to a JSON reader.** `current_results` is used for sorting, `claim` and the "nobody has tested this commit" line, but `list --json`/`show --json` (`public(t)`) return the raw `results`. The /testing skill tells the agent to skip a request when its own name is in `results`, "unless the PR changed". After `update`, an agent doing that will skip the card it should now test, because its old result is still listed. Please expose the filtered view (e.g. a `current` flag on each result, or `current_results` as its own key) and word the skill step in terms of it. The same "unless the PR changed since your result" wording is still in both skills and in `claim`'s refusal path: with pins, a moved head without an `update` does not make the same pin worth retesting, so it should say "unless the card was updated to a new commit". 2. **`--commit` is not tied to the PR, and that sits next to the approval gate.** The guide says approval keeps strangers' code off testers' PCs until someone has looked at the PR. Now any requester can pin an arbitrary 7-40 hex value, and `git checkout --detach <commit>` in the testing skill will build it if it is fetchable (GitHub serves commits from other forks in the same network by SHA). The PR calls this out ("membership in the PR is not checked") but the tester side has no guard. Cheapest fix is in the testing skill: before building, check the commit is part of the PR (`git merge-base --is-ancestor <commit> <PR head>`, or the PR's commit list), review that commit's diff rather than the head's, and release the claim if it is not. Checking it in `request`/`update` through the PR commits endpoint would be stronger but costs a call; I would be fine with the skill-only version. 3. **A moved head still lets the claim go through.** `claim` posts, prints "ask the requester to confirm", and the tester is then holding a 90 minute claim on a commit that may already be superseded. For `/loop 30m /testing` there is nobody to ask. `list` already shows the "head moved" line, so the skill could say to skip cards that show it unless the requester has confirmed, or `claim` could need an explicit `--commit` when the head moved. Your call, but as written an unattended agent will happily test the stale pin. 4. **`request` now needs GitHub.** Before, a request posted even when GitHub was down or rate limited (60 an hour without a token, shared by an office or a CGNAT). Now it fails unless an explicit full SHA is given. The error says so, so I think that is acceptable, but a one-line mention in the request skill next to `GITHUB_TOKEN` would help, since the 40-character fallback needs a `git rev-parse` the agent may not think of. Nits: - `claim_commits` is not cleared on `release` or expiry, so a `done` with no new `claim` silently uses the old claim's commit. Dropping it on those two events (or requiring `--commit` when the claim is gone) is cleaner. - Notes lines starting with `Commit:` or `Head:` are swallowed by the request parser. Harmless in practice. - `docs/test-requests.md` will conflict with #2807 as you said; whoever lands second rebases. Relay: only the request message was trialled in Discord. The claim, result and `update` messages (and an unknown `update` action) have not been seen by the relay yet, so that check is still open before this is relied on. I would not merge until it is done. Nothing here depends on game or GPU measurements, so no in-game evidence is needed.

github-sync force-pushed github/pr/2830 from d22aec74f8 to daad135081

2026-11-12 19:01:02 +00:00

Compare

github-sync commented

2026-11-12 19:02:37 +00:00

Author

Owner

@mooki000 commented on GitHub · 2026-11-12 18:01 UTC · original

Thanks for the careful read. All four points are right, and I am taking them as you ordered them:

A current view in the JSON output, with both skills and claim's refusal worded in terms of it, so an agent no longer skips a card because of a result for an older commit.
The pinned commit tied to the pull request: the testing skill checks that it belongs to the pull request before anything is built, reviews that commit's diff, and lets the claim go if it does not belong; and request refuses a commit outside the pull request when GitHub can tell.
A moved head no longer lets an unattended claim through.
The line about GitHub in the request skill, with the git rev-parse for the full hash.

The nits too: the claim's commit dropped on release and expiry, and note lines that start with Commit: or Head:.

The relay: agreed that it should not merge before the claim, result and update messages have gone through the real room. I will run them on the trial card once the fixes are pushed and ask you to look at how the relay shows them. If the card cannot be claimed without the label, I will ask you for the label on this pull request for that one trial.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 18:01 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2830#issuecomment-6831709706) Thanks for the careful read. All four points are right, and I am taking them as you ordered them: 1. **A `current` view in the JSON output**, with both skills and `claim`'s refusal worded in terms of it, so an agent no longer skips a card because of a result for an older commit. 2. **The pinned commit tied to the pull request**: the testing skill checks that it belongs to the pull request before anything is built, reviews that commit's diff, and lets the claim go if it does not belong; and `request` refuses a commit outside the pull request when GitHub can tell. 3. **A moved head no longer lets an unattended claim through.** 4. **The line about GitHub** in the request skill, with the `git rev-parse` for the full hash. The nits too: the claim's commit dropped on release and expiry, and note lines that start with `Commit:` or `Head:`. **The relay:** agreed that it should not merge before the claim, result and `update` messages have gone through the real room. I will run them on the trial card once the fixes are pushed and ask you to look at how the relay shows them. If the card cannot be claimed without the label, I will ask you for the label on this pull request for that one trial.

github-sync commented

2026-11-12 19:02:37 +00:00

Author

Owner

@mooki000 commented on GitHub · 2026-11-12 18:41 UTC · original

Thanks; each point is now addressed:

56661df6: JSON exposes current_results; both skills and claim refusals distinguish the card's pin from a moved PR head.
8652d4b8: request/update verify non-head commits with GitHub. Before building, verify checks the fetched PR ancestry and prints the pinned diff; an outside commit releases the claim. A real Git fixture covers a fetchable outside commit.
6849b187: a moved head sends no claim without explicit --commit confirmation; unattended loops must skip it. Verification also stops if the head moves after claiming.
3a8262d2: the request skill puts git rev-parse --verify "HEAD^{commit}" beside the token/outage instructions.
Nits: 8db228ee clears commit/head snapshots when claims end; daad1350 preserves metadata-looking note lines and documents the reserved trailing footer.
Rebased cleanly onto 4cf785a0; head daad1350. No conflicts; all seven patches unchanged in the final rebase.
Checks: 270 offline checks pass, including the real old client. The whole Linux suite: 501 passed, 1 failed, 20 skipped; only main's known test_readme_menu_rows.py failure (missing “Skip opening”). Pre-push check: PASS.
A full run of the project's checks on this exact head in my fork, Windows included: tests/test_test_requests.py passes on Ubuntu and Windows. Red there were only tests this branch does not touch: main's two known ones (test_readme_menu_rows.py, and on Windows test_wb_shadow_remake.py, fix in #2868), plus one Windows failure of test_recorder_sync.py whose own output shows a timing-dependent negative case ("vkDestroyBuffer made without waiting ... NOT CAUGHT"); I will look at that one separately.
The live claim/update/result trial is still pending. Could you add testing wanted to #2830 for T-6BH3? I will update its old pin to the reviewed head first, then capture each relay step; no game run or unrelated PR is needed.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 18:41 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2830#issuecomment-6832639364) Thanks; each point is now addressed: 1. `56661df6`: JSON exposes `current_results`; both skills and claim refusals distinguish the card's pin from a moved PR head. 2. `8652d4b8`: request/update verify non-head commits with GitHub. Before building, `verify` checks the fetched PR ancestry and prints the pinned diff; an outside commit releases the claim. A real Git fixture covers a fetchable outside commit. 3. `6849b187`: a moved head sends no claim without explicit `--commit` confirmation; unattended loops must skip it. Verification also stops if the head moves after claiming. 4. `3a8262d2`: the request skill puts `git rev-parse --verify "HEAD^{commit}"` beside the token/outage instructions. Nits: `8db228ee` clears commit/head snapshots when claims end; `daad1350` preserves metadata-looking note lines and documents the reserved trailing footer. Rebased cleanly onto `4cf785a0`; head `daad1350`. No conflicts; all seven patches unchanged in the final rebase. Checks: 270 offline checks pass, including the real old client. The whole Linux suite: 501 passed, 1 failed, 20 skipped; only main's known `test_readme_menu_rows.py` failure (missing “Skip opening”). Pre-push check: PASS. A full run of the project's checks on this exact head in my fork, Windows included: `tests/test_test_requests.py` passes on Ubuntu and Windows. Red there were only tests this branch does not touch: main's two known ones (`test_readme_menu_rows.py`, and on Windows `test_wb_shadow_remake.py`, fix in #2868), plus one Windows failure of `test_recorder_sync.py` whose own output shows a timing-dependent negative case ("vkDestroyBuffer made without waiting ... NOT CAUGHT"); I will look at that one separately. The live claim/update/result trial is still pending. Could you add `testing wanted` to #2830 for T-6BH3? I will update its old pin to the reviewed head first, then capture each relay step; no game run or unrelated PR is needed.

Commenting is not possible because the repository is archived.

No reviewers