Skip to content

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

Open
mooki000 wants to merge 1 commit into
NotRexed:mainfrom
mooki000:feat/test-requests-pin-commit
Open

mooki000 wants to merge 1 commit into
NotRexed:mainfrom
mooki000:feat/test-requests-pin-commit

Conversation

@mooki000

@mooki000 mooki000 commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

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://github.com/NotRexed/Hookshot/pull/2731#issuecomment-123
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://github.com/NotRexed/Hookshot/pull/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://github.com/NotRexed/Hookshot/pull/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:

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra noreply@openai.com

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 <noreply@openai.com>
@mooki000
mooki000 marked this pull request as ready for review November 12, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant