MagicBeanDev/Hookshot
Archived
Watch
2
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

ci: label-gated hardware tests on testers' self-hosted runners, through the owner's sandbox #2807

Open
github-sync wants to merge 4 commits from github/pr/2807 into main
pull from: github/pr/2807
merge into: MagicBeanDev:main
Conversation 5 Commits 4 Files changed 6 +753 -1
github-sync commented 2026-11-12 15:48:58 +00:00
Owner

@Joey-Magnus opened this pull request on GitHub · 2026-11-12 14:50 UTC · original

Testers want PRs tested on their own hardware automatically (#2797), and Dr.Crow suggested starting those tests with a label. This PR adds that trigger, with security rules a PR can't get around, so runners can be added later without exposing testers' PCs or their game dumps.

For now it's a fork-local pilot. The label, the PR being tested and the runner are all on one tester's fork: the PR is a test PR opened on the fork itself. The workflows dispatch, check and report in the repository they run in, with its own token, so a hardware: label on a NotRexed/Hookshot PR does not reach a fork's runner (here it only leaves a notice), and registering runners on NotRexed/Hookshot is not the fix. Testing upstream PRs automatically would need an authorized relay, which is not built.

What it adds

  • .github/workflows/hardware-test-label.yml (the gate). When a maintainer adds a hardware: <name> label (for example hardware: steam-deck), it starts hardware-test.yml from main, for the exact commit that was labelled, but only if <name> is listed in the repository's HOOKSHOT_HW_RUNNERS variable; otherwise it leaves a notice pointing at the fork pilot. New commits take every hardware: label off, with a comment, so a maintainer looks again before the next run. A cleanup that runs late does nothing if a newer push came since, and leaves a label that was put on after its push. It never checks out the PR. That's why pull_request_target is safe here, the same reasoning as pr-title.yml.
  • .github/workflows/hardware-test.yml (workflow_dispatch only, from the default branch only):
    • check (GitHub's runner): the commit must still be the PR's head and still carry the label.
    • run (the tester's Linux runner, [self-hosted, linux, hookshot-hardware, <name>], Bash): permissions: {} and no checkout. After the queue it checks again, through the public API, that the PR is open, labelled and at that commit, and stops if not. Then it calls the owner's sandbox program (HOOKSHOT_HW_SANDBOX) and refuses to run if there isn't one. The sandbox fetches the commit, cuts the network, builds and runs with the dump read-only, and writes only results.txt and screenshots.
    • report (GitHub's runner): posts the outcome and results.txt on the PR as quoted text. The runner's text can't close the code fence or @mention anyone, and it's cut at 60,000 bytes after escaping, so the comment stays under GitHub's limit. It never runs anything the runner produced.
  • docs/hardware-runners.md: the fork-local pilot, how a test starts, and what a runner owner must set up: a throwaway VM, the approval checked again before the dump goes in and before taking the GPU, no network after fetching the commit, the dump read-only, an exporter that puts only capped regular files in --out, no credentials (the runner's own session credentials stay outside the workload), and ephemeral runners. It's linked from docs/test-requests.md.
  • tests/test_hardware_test_workflow.py: game-free checks that keep those rules in place:
    • Only a dispatch starts the test, and only that workflow uses a self-hosted runner, with no token and no checkout.
    • No ${{ }} inside any shell script, and every action is pinned to a commit.
    • It also runs the real shell steps with a stand-in gh and curl. Bad labels (x$(id), spaces, a;b, too long) and hardware the repository has no runner for dispatch nothing. A label like bug doesn't count as hardware bug. A moved head, a missing label or a manual run from another branch stops the run, and so does a closed PR, a moved head or a removed label found after the queue. A late cleanup leaves newer labels and newer pushes alone. Injected fences and @mentions are neutralized, 60,000 @ bytes stay under the limit, and a file without a final newline still closes the fence.
  • .github/actionlint.yaml: declares the custom runner label, so actionlint passes.

Not in this PR

  • No runners are registered, and nothing changes until one is. The pilot registers a runner on a tester's fork and tests a fork-local PR. A runner on this repository could be picked by a PR's own pull_request workflow, past the label and the sandbox (mooki's research comment on #2797). Putting them here would need runner groups limited to this workflow, which a personal account's repository doesn't have, and is the maintainers' call.
  • No upstream-to-fork relay. See above.
  • No reference sandbox. mooki000 is writing a Linux one to the contract in the doc; the GPU side and a real pilot run are still to come.
  • The hardware: labels still need creating, one per runner type, on the fork that runs the pilot.

Checks

  • python -I -B tests/run_all.py: everything passes except test_lift_cache and test_updater_asset_digest, which fail the same way on main on this machine. The new test passes (31 checks).
  • actionlint 1.7.12 and shellcheck 0.11.0 on both workflows: clean.
  • Nothing shows on screen, so no screenshots.
> [@Joey-Magnus](https://github.com/Joey-Magnus) opened this pull request on GitHub · 2026-11-12 14:50 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807) Testers want PRs tested on their own hardware automatically ([#2797](https://git.magicbean.dev/MagicBeanDev/Hookshot/issues/2797)), and Dr.Crow suggested starting those tests with a label. This PR adds that trigger, with security rules a PR can't get around, so runners can be added later without exposing testers' PCs or their game dumps. **For now it's a fork-local pilot.** The label, the PR being tested and the runner are all on one tester's fork: the PR is a test PR opened on the fork itself. The workflows dispatch, check and report in the repository they run in, with its own token, so a `hardware:` label on a NotRexed/Hookshot PR does not reach a fork's runner (here it only leaves a notice), and registering runners on NotRexed/Hookshot is not the fix. Testing upstream PRs automatically would need an authorized relay, which is not built. ## What it adds - **`.github/workflows/hardware-test-label.yml`** (the gate). When a maintainer adds a `hardware: <name>` label (for example `hardware: steam-deck`), it starts `hardware-test.yml` **from `main`**, for the exact commit that was labelled, but only if `<name>` is listed in the repository's `HOOKSHOT_HW_RUNNERS` variable; otherwise it leaves a notice pointing at the fork pilot. New commits take every `hardware:` label off, with a comment, so a maintainer looks again before the next run. A cleanup that runs late does nothing if a newer push came since, and leaves a label that was put on after its push. It never checks out the PR. That's why `pull_request_target` is safe here, the same reasoning as `pr-title.yml`. - **`.github/workflows/hardware-test.yml`** (`workflow_dispatch` only, from the default branch only): - `check` (GitHub's runner): the commit must still be the PR's head and still carry the label. - `run` (the tester's Linux runner, `[self-hosted, linux, hookshot-hardware, <name>]`, Bash): `permissions: {}` and no checkout. After the queue it checks again, through the public API, that the PR is open, labelled and at that commit, and stops if not. Then it calls the owner's sandbox program (`HOOKSHOT_HW_SANDBOX`) and refuses to run if there isn't one. The sandbox fetches the commit, cuts the network, builds and runs with the dump read-only, and writes only `results.txt` and screenshots. - `report` (GitHub's runner): posts the outcome and `results.txt` on the PR as quoted text. The runner's text can't close the code fence or @mention anyone, and it's cut at 60,000 bytes after escaping, so the comment stays under GitHub's limit. It never runs anything the runner produced. - **`docs/hardware-runners.md`**: the fork-local pilot, how a test starts, and what a runner owner must set up: a throwaway VM, the approval checked again before the dump goes in and before taking the GPU, no network after fetching the commit, the dump read-only, an exporter that puts only capped regular files in `--out`, no credentials (the runner's own session credentials stay outside the workload), and ephemeral runners. It's linked from `docs/test-requests.md`. - **`tests/test_hardware_test_workflow.py`**: game-free checks that keep those rules in place: - Only a dispatch starts the test, and only that workflow uses a self-hosted runner, with no token and no checkout. - No `${{ }}` inside any shell script, and every action is pinned to a commit. - It also runs the real shell steps with a stand-in `gh` and `curl`. Bad labels (`x$(id)`, spaces, `a;b`, too long) and hardware the repository has no runner for dispatch nothing. A label like `bug` doesn't count as hardware `bug`. A moved head, a missing label or a manual run from another branch stops the run, and so does a closed PR, a moved head or a removed label found after the queue. A late cleanup leaves newer labels and newer pushes alone. Injected fences and @mentions are neutralized, 60,000 `@` bytes stay under the limit, and a file without a final newline still closes the fence. - **`.github/actionlint.yaml`**: declares the custom runner label, so `actionlint` passes. ## Not in this PR - **No runners are registered**, and nothing changes until one is. The pilot registers a runner on a tester's fork and tests a fork-local PR. A runner on this repository could be picked by a PR's own `pull_request` workflow, past the label and the sandbox (mooki's research comment on #2797). Putting them here would need runner groups limited to this workflow, which a personal account's repository doesn't have, and is the maintainers' call. - **No upstream-to-fork relay.** See above. - **No reference sandbox.** mooki000 is writing a Linux one to the contract in the doc; the GPU side and a real pilot run are still to come. - **The `hardware:` labels still need creating**, one per runner type, on the fork that runs the pilot. ## Checks - `python -I -B tests/run_all.py`: everything passes except `test_lift_cache` and `test_updater_asset_digest`, which fail the same way on `main` on this machine. The new test passes (31 checks). - `actionlint` 1.7.12 and `shellcheck` 0.11.0 on both workflows: clean. - Nothing shows on screen, so no screenshots. <!-- github-sync:item:2807 -->
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
github-sync commented 2026-11-12 16:56:23 +00:00
Author
Owner

@mooki000 reviewed on GitHub · 2026-11-12 15:54 UTC · original

Thanks for turning the label idea into a pull request this fast. I reviewed ff236c98 as the workflow the fork pilot will run under. Four findings, the first one the most practical:

  1. The upstream-to-fork route is missing. hardware-test-label.yml:28,40 dispatches in github.repository, so an upstream label starts an upstream run. Its runner job cannot use a repository runner registered on a tester's fork. Manually dispatching the fork with the upstream PR number also does not bridge it: hardware-test.yml:49,57,82,110,133 checks, fetches and reports in the fork. A fork-local test PR works; automatic testing of an upstream PR needs an authorized relay and separate execution-repository/source-PR identities. The default token is repository-scoped. Please either implement that route or document the fork-local-only pilot explicitly; registering runners upstream is not the fix.

  2. Recheck approval after the runner queue. hardware-test.yml:57–61 checks the head/label before the hardware job can wait up to 24 hours; 77–82 then calls the sandbox without rechecking. A removed label, closed PR or moved head during the wait is not caught. The sandbox contract should require a trusted fail-closed recheck before exposing the dump, and again before GPU acquisition after a long build. This is stale approval for the old pinned SHA, not substitution of an unreviewed new SHA.

  3. A delayed synchronization job can remove a newer approval. hardware-test-label.yml:58–61 reads and deletes live labels. A queued cleanup for push A can remove a label added after review of B—or after review of the same head while cleanup was delayed. Skip obsolete-head events and distinguish label applications newer than the triggering push; an event-SHA check alone does not cover the same-head case.

  4. Cap the escaped comment, not just its input. At hardware-test.yml:126, escaping expands the bytes after head -c 60000. With a fake gh, 60,000 @ bytes produced 120,268 comment characters / 240,268 UTF-8 bytes including the wrapper. Reserve space for the wrapper and cap after escaping. Also add an explicit newline before the closing fence: a file without a final newline currently joins that fence to its content. No API comment or rendered-Markdown test was performed.

The ordinary fence/mention case is handled, and I found no label/branch-name shell injection: environment variables, quoted expansions and validation before jq interpolation are the right pattern. The reporter does not execute the result. The missing actions: read permission is not a demonstrated problem for the downloader's same-run mode.

Two contract clarifications: permissions: {} removes configurable repository-token permissions, not the runner's own session/artifact credentials; those must remain outside the workload. Also, the full results.txt/PNG artifact uploads before comment truncation, without size/type checks, and the pinned uploader follows symlinks. The trusted owner-side exporter must enforce regular files, content allow-listing and byte/count caps. No-network execution does not remove this publication channel.

The tests cover the immediate snapshot gate and a short report, but not repository identity (GH_REPO), synchronization races, acquisition-time revocation, output expansion or sandbox/export behavior. A manual default-branch guard and an explicit Linux/Bash scope would also make the first pilot clearer.

Where the pilot stands: I have a draft of a Linux reference program written to this invocation contract (it exports only bounded, regenerated text at first). It stays fail-closed for now: the existing local GPU guard needs cross-user/namespace-aware integration, and the build gate must be entered from a trusted neutral directory so its relative shader hook cannot execute PR code outside confinement. Nothing is registered and no game has been started under it; the cases I ran against it are authored fakes, not a sandbox or GPU proof. For the pilot itself I will use the route that works as written: a fork-local test pull request on my fork. I will tag you when the program and the workflow run are up.

> [@mooki000](https://github.com/mooki000) reviewed on GitHub · 2026-11-12 15:54 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807#pullrequestreview-5877583821) Thanks for turning the label idea into a pull request this fast. I reviewed `ff236c98` as the workflow the fork pilot will run under. Four findings, the first one the most practical: 1. **The upstream-to-fork route is missing.** `hardware-test-label.yml:28,40` dispatches in `github.repository`, so an upstream label starts an upstream run. Its runner job cannot use a repository runner registered on a tester's fork. Manually dispatching the fork with the upstream PR number also does not bridge it: `hardware-test.yml:49,57,82,110,133` checks, fetches and reports in the fork. A **fork-local test PR** works; automatic testing of an upstream PR needs an authorized relay and separate execution-repository/source-PR identities. The default token is [repository-scoped](https://docs.github.com/en/actions/concepts/security/github_token). Please either implement that route or document the fork-local-only pilot explicitly; registering runners upstream is not the fix. 2. **Recheck approval after the runner queue.** `hardware-test.yml:57–61` checks the head/label before the hardware job can wait up to 24 hours; `77–82` then calls the sandbox without rechecking. A removed label, closed PR or moved head during the wait is not caught. The sandbox contract should require a trusted fail-closed recheck before exposing the dump, and again before GPU acquisition after a long build. This is stale approval for the **old pinned SHA**, not substitution of an unreviewed new SHA. 3. **A delayed synchronization job can remove a newer approval.** `hardware-test-label.yml:58–61` reads and deletes live labels. A queued cleanup for push A can remove a label added after review of B—or after review of the same head while cleanup was delayed. Skip obsolete-head events and distinguish label applications newer than the triggering push; an event-SHA check alone does not cover the same-head case. 4. **Cap the escaped comment, not just its input.** At `hardware-test.yml:126`, escaping expands the bytes after `head -c 60000`. With a fake `gh`, 60,000 `@` bytes produced **120,268 comment characters / 240,268 UTF-8 bytes** including the wrapper. Reserve space for the wrapper and cap after escaping. Also add an explicit newline before the closing fence: a file without a final newline currently joins that fence to its content. No API comment or rendered-Markdown test was performed. The ordinary fence/mention case is handled, and I found no label/branch-name shell injection: environment variables, quoted expansions and validation before jq interpolation are the right pattern. The reporter does not execute the result. The missing `actions: read` permission is not a demonstrated problem for the downloader's same-run mode. Two contract clarifications: `permissions: {}` removes configurable repository-token permissions, not the runner's own session/artifact credentials; those must remain outside the workload. Also, the full `results.txt`/PNG artifact uploads **before** comment truncation, without size/type checks, and the [pinned uploader follows symlinks](https://github.com/actions/upload-artifact/blob/cf430e030ddbb5b0abf93d22962f4752f3646cd9/src/shared/search.ts#L14-L21). The trusted owner-side exporter must enforce regular files, content allow-listing and byte/count caps. No-network execution does not remove this publication channel. The tests cover the immediate snapshot gate and a short report, but not repository identity (`GH_REPO`), synchronization races, acquisition-time revocation, output expansion or sandbox/export behavior. A manual default-branch guard and an explicit Linux/Bash scope would also make the first pilot clearer. **Where the pilot stands:** I have a draft of a Linux reference program written to this invocation contract (it exports only bounded, regenerated text at first). It stays **fail-closed** for now: the existing local GPU guard needs cross-user/namespace-aware integration, and the build gate must be entered from a trusted neutral directory so its relative shader hook cannot execute PR code outside confinement. Nothing is registered and no game has been started under it; the cases I ran against it are authored fakes, not a sandbox or GPU proof. For the pilot itself I will use the route that works as written: a fork-local test pull request on my fork. I will tag you when the program and the workflow run are up. <!-- github-sync:r:5877583821 -->
No conflicts.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-authored-by: Cursor <[email protected]>
The pilot is fork-local only: the label, the test pull request and the runner are all on one tester's fork. The
workflow headers and docs/hardware-runners.md say so, and that upstream labels do not reach a fork's runner and
registering runners upstream is not the fix. The gate dispatches only for hardware names listed in the repository's
HOOKSHOT_HW_RUNNERS variable; without it a label leaves a notice and starts nothing.

The runner job checks again, after the queue and before calling the sandbox, that the pull request is open, still
carries the label and is still at the pinned commit, through the public API (the runner has no token), and stops on
anything else. The sandbox contract asks for the same check before the dump goes in and before taking the GPU.

The label cleanup does nothing when its push is no longer the head, and keeps a label whose latest labeled event is
newer than the push; if the label events cannot be read, every label comes off.

The report escapes first and caps after, at 60,000 bytes, drops a character the cut split, puts a newline before
the closing fence and says when it cut. This also fixes actionlint's SC2016 on that step.

Manual runs must come from the default branch; the runner job is Linux and Bash. The doc adds that the exporter
puts only capped regular files in --out, and that the runner's own credentials stay outside the workload.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-authored-by: Cursor <[email protected]>
github-sync commented 2026-11-12 17:59:31 +00:00
Author
Owner

@Joey-Magnus commented on GitHub · 2026-11-12 17:02 UTC · original

Thanks @mooki000, all four points are addressed in 31e67594, on top of a merge of main (59f5eaed, no conflicts).

1. Upstream-to-fork route. No relay: the pilot is now explicitly fork-local. The headers of both workflows, a new "For now: a pilot on one fork" section in docs/hardware-runners.md and the PR description say that the label, the test PR and the runner are all on one tester's fork, that an upstream label does not reach a fork's runner, that dispatching the fork by hand with an upstream PR number does not bridge it, and that registering runners upstream is not the fix. The gate also dispatches only for hardware names listed in the repository's HOOKSHOT_HW_RUNNERS variable, so on NotRexed/Hookshot (no variable) a label only leaves a notice pointing at the fork pilot instead of queueing a run for 24 hours.

2. Recheck after the queue. The runner job now has a first step, before the sandbox, that reads the PR again through the public API (the runner still has no token) and stops unless it is open, still at the pinned SHA and still labelled hardware: <name>; an unreadable or unexpected answer stops it too. The contract in the doc now requires the sandbox to repeat that check from outside the sandbox before the dump goes in and again before taking the GPU, and passes the hardware name as HOOKSHOT_HW_HARDWARE (an environment variable, so the command line your draft follows is unchanged).

3. Delayed cleanup. The cleanup reads the head and labels in one call and does nothing (with a notice) if its push is no longer the head. Otherwise it reads the issue events and keeps any label whose latest labeled event is newer than the push (pull_request.updated_at from the synchronize payload), which covers the same-head case. If the events can't be read, every label comes off, the safe side. It comments only when it removed something.

4. Comment cap. The report escapes first, then cuts to 60,000 bytes, drops a UTF-8 character the cut split, always puts a newline before the closing fence, and says when it cut. With the stand-in gh, 60,000 @ bytes now give a 30,324-character (60,324-byte) comment with exactly two fences, down from 120,268. This also fixes the actionlint failure (SC2016 on that step).

Clarifications. Manual dispatch from any branch but the default one stops in check. The runner job is [self-hosted, linux, hookshot-hardware, <name>] with shell: bash, and the doc says runners are Linux and need curl and jq. The contract now says the owner-side exporter puts only regular files in --out (no symlinks, since the uploader follows them), only results.txt and real PNGs, with size and count caps, and that permissions: {} does not remove the runner's own session and artifact credentials, so the workload starts in a fresh environment as another user.

Tests. tests/test_hardware_test_workflow.py went from 16 to 31 checks. The new ones run the real steps with stand-ins: the runner notice, the after-queue recheck (closed, moved head, removed or look-alike label, failed or junk response), the default-branch guard, the cleanup races (an older push, a label put on after the push, unreadable events), 60,000 @ bytes with and without a split character, repeated fences, a file with no final newline and an empty file. I checked each new check fails when its fix is reverted. actionlint 1.7.12 and shellcheck 0.11.0 are clean on both workflows. The full tests/run_all.py passes except test_lift_cache and test_updater_asset_digest, which fail the same way on main on this machine.

Still open. The sandbox and the GPU side are your work in progress, and so is a real pilot run on a runner: nothing here has run on a self-hosted runner, against a real sandbox, or posted a real comment through the API. No upstream relay, by design for now.

(_trash.panda's agent)

> [@Joey-Magnus](https://github.com/Joey-Magnus) commented on GitHub · 2026-11-12 17:02 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807#issuecomment-6830288534) Thanks @mooki000, all four points are addressed in 31e67594, on top of a merge of `main` (59f5eaed, no conflicts). **1. Upstream-to-fork route.** No relay: the pilot is now explicitly fork-local. The headers of both workflows, a new "For now: a pilot on one fork" section in `docs/hardware-runners.md` and the PR description say that the label, the test PR and the runner are all on one tester's fork, that an upstream label does not reach a fork's runner, that dispatching the fork by hand with an upstream PR number does not bridge it, and that registering runners upstream is not the fix. The gate also dispatches only for hardware names listed in the repository's `HOOKSHOT_HW_RUNNERS` variable, so on NotRexed/Hookshot (no variable) a label only leaves a notice pointing at the fork pilot instead of queueing a run for 24 hours. **2. Recheck after the queue.** The runner job now has a first step, before the sandbox, that reads the PR again through the public API (the runner still has no token) and stops unless it is open, still at the pinned SHA and still labelled `hardware: <name>`; an unreadable or unexpected answer stops it too. The contract in the doc now requires the sandbox to repeat that check from outside the sandbox before the dump goes in and again before taking the GPU, and passes the hardware name as `HOOKSHOT_HW_HARDWARE` (an environment variable, so the command line your draft follows is unchanged). **3. Delayed cleanup.** The cleanup reads the head and labels in one call and does nothing (with a notice) if its push is no longer the head. Otherwise it reads the issue events and keeps any label whose latest `labeled` event is newer than the push (`pull_request.updated_at` from the synchronize payload), which covers the same-head case. If the events can't be read, every label comes off, the safe side. It comments only when it removed something. **4. Comment cap.** The report escapes first, then cuts to 60,000 bytes, drops a UTF-8 character the cut split, always puts a newline before the closing fence, and says when it cut. With the stand-in `gh`, 60,000 `@` bytes now give a 30,324-character (60,324-byte) comment with exactly two fences, down from 120,268. This also fixes the actionlint failure (SC2016 on that step). **Clarifications.** Manual dispatch from any branch but the default one stops in `check`. The runner job is `[self-hosted, linux, hookshot-hardware, <name>]` with `shell: bash`, and the doc says runners are Linux and need `curl` and `jq`. The contract now says the owner-side exporter puts only regular files in `--out` (no symlinks, since the uploader follows them), only `results.txt` and real PNGs, with size and count caps, and that `permissions: {}` does not remove the runner's own session and artifact credentials, so the workload starts in a fresh environment as another user. **Tests.** `tests/test_hardware_test_workflow.py` went from 16 to 31 checks. The new ones run the real steps with stand-ins: the runner notice, the after-queue recheck (closed, moved head, removed or look-alike label, failed or junk response), the default-branch guard, the cleanup races (an older push, a label put on after the push, unreadable events), 60,000 `@` bytes with and without a split character, repeated fences, a file with no final newline and an empty file. I checked each new check fails when its fix is reverted. `actionlint` 1.7.12 and `shellcheck` 0.11.0 are clean on both workflows. The full `tests/run_all.py` passes except `test_lift_cache` and `test_updater_asset_digest`, which fail the same way on `main` on this machine. **Still open.** The sandbox and the GPU side are your work in progress, and so is a real pilot run on a runner: nothing here has run on a self-hosted runner, against a real sandbox, or posted a real comment through the API. No upstream relay, by design for now. (_trash.panda's agent) <!-- github-sync:c:6830288534 -->
github-sync commented 2026-11-12 17:59:31 +00:00
Author
Owner

@mooki000 reviewed on GitHub · 2026-11-12 17:07 UTC · original

Thanks for the quick turn. I read 31e67594 whole (both workflows) and ran tests/test_hardware_test_workflow.py at that commit on Linux: 31 pass (bash 5.3; jq on this PC is jaq 3.1, so the after-queue filter works with that too). All four points are closed as I read them:

  1. Fork-local. The gate dispatches only for names in HOOKSHOT_HW_RUNNERS, so a label here leaves a notice and queues nothing; the headers and the doc say where the pilot lives.
  2. After the queue. The runner's first step stops unless the pull request is open, at the pinned commit and still labelled, and on any answer it cannot read; the contract has the sandbox repeat it.
  3. Cleanup. Head and labels come from one read, a label whose latest labeled event is after the push stays, and unreadable events take every label off.
  4. Comment. Escape, then cut, then drop a split character, newline before the fence, and a note when cut. I also tried runs of five and seven backticks through the escape: no line is left that could close the fence.

Two small observations, neither blocking:

  • Cleanup when the head has moved again (the head != SHA early exit in the label workflow): the older push's run leaves everything to the newer push's run. If that one fails on a read or never starts, a label from before both pushes stays on. The timestamp rule already makes it safe for the older run to carry on, since it only takes off labels put on before its own push, and those are older than any later push too; so dropping the early exit loses nothing. The harm today is small either way: only a labeled event dispatches, and the after-queue check compares the head.
  • The recheck without a token draws on the public API's allowance for anonymous reads (60 an hour per address), and the contract has the sandbox read twice more. Stopping when it cannot read is right; one line in the doc would save a tester wondering why a run stopped with "could not read" after a busy hour.

On my side: the Linux reference sandbox is being brought to the contract at this head (HOOKSHOT_HW_HARDWARE, the two repeat checks from outside the sandbox, regular files only in --out), with tests for the hostile cases. It goes on #2797 when those pass, and no runner gets registered before then.

> [@mooki000](https://github.com/mooki000) reviewed on GitHub · 2026-11-12 17:07 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807#pullrequestreview-5878725295) Thanks for the quick turn. I read `31e67594` whole (both workflows) and ran `tests/test_hardware_test_workflow.py` at that commit on Linux: **31 pass** (bash 5.3; `jq` on this PC is jaq 3.1, so the after-queue filter works with that too). All four points are closed as I read them: 1. **Fork-local.** The gate dispatches only for names in `HOOKSHOT_HW_RUNNERS`, so a label here leaves a notice and queues nothing; the headers and the doc say where the pilot lives. 2. **After the queue.** The runner's first step stops unless the pull request is open, at the pinned commit and still labelled, and on any answer it cannot read; the contract has the sandbox repeat it. 3. **Cleanup.** Head and labels come from one read, a label whose latest `labeled` event is after the push stays, and unreadable events take every label off. 4. **Comment.** Escape, then cut, then drop a split character, newline before the fence, and a note when cut. I also tried runs of five and seven backticks through the escape: no line is left that could close the fence. Two small observations, neither blocking: - **Cleanup when the head has moved again** (the `head != SHA` early exit in the label workflow): the older push's run leaves everything to the newer push's run. If that one fails on a read or never starts, a label from before both pushes stays on. The timestamp rule already makes it safe for the older run to carry on, since it only takes off labels put on before its own push, and those are older than any later push too; so dropping the early exit loses nothing. The harm today is small either way: only a `labeled` event dispatches, and the after-queue check compares the head. - **The recheck without a token** draws on the public API's allowance for anonymous reads (60 an hour per address), and the contract has the sandbox read twice more. Stopping when it cannot read is right; one line in the doc would save a tester wondering why a run stopped with "could not read" after a busy hour. On my side: the Linux reference sandbox is being brought to the contract at this head (`HOOKSHOT_HW_HARDWARE`, the two repeat checks from outside the sandbox, regular files only in `--out`), with tests for the hostile cases. It goes on #2797 when those pass, and no runner gets registered before then. <!-- github-sync:r:5878725295 -->
github-sync commented 2026-11-12 17:59:31 +00:00
Author
Owner

@Nitnek reviewed on GitHub · 2026-11-12 17:14 UTC · original

I independently replayed the six changed files at 31e67594f1 on Linux: python3 -I -B tests/test_hardware_test_workflow.py returned 31 tests, OK, exit 0. This was a game-free replay with the fixture's local gh/curl stand-ins, not a self-hosted runner, sandbox or GPU run. The assertion about other workflows is limited to the collected workflow files, not a full-repository security approval. I am not repeating mooki's findings on the older revision as if they were still open.

One small result-contract follow-up for the pilot, separate from the workflow corrections: keep orchestration outcome and functional verdict explicitly separate. I also replayed the current Comment script with a recording gh double and OUTCOME=success:

  • results.txt containing SYNTHETIC required check scene_complete: FAIL produces the **success** header and preserves the FAIL text below it.
  • No results.txt produces the same **success** header followed by The runner wrote no results.txt.

That is consistent with the documented job-outcome reporting; I am not calling it a demonstrated false hardware PASS or a lost failure. It does mean that a consumer must not turn the bold job status into a test verdict.

For a later machine-readable receipt, a conservative rule is: validate repository/PR/exact SHA/hardware/scene identity and schema first; take required check names from a separate trusted plan; a known workload failure, fatal/device-loss observation or required FAIL dominates incomplete evidence; missing/SKIPPED/UNKNOWN checks stay INCONCLUSIVE; PASS requires every named required check PASS and complete, clean process evidence. That is still an untrusted self-report, not authentication or isolation. The report should never execute receipt content.

No runner registration, workflow dispatch, real game data, sandbox qualification or GPU result is implied by these replays. This is a COMMENT-level verification/follow-up, not a general security approval.

— Nitnek, with AI-assisted analysis

> [@Nitnek](https://github.com/Nitnek) reviewed on GitHub · 2026-11-12 17:14 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807#pullrequestreview-5878837942) I independently replayed the six changed files at **31e67594f173b1f20403342af3eb6587625b00c2** on Linux: `python3 -I -B tests/test_hardware_test_workflow.py` returned **31 tests, OK, exit 0**. This was a game-free replay with the fixture's local `gh`/`curl` stand-ins, not a self-hosted runner, sandbox or GPU run. The assertion about other workflows is limited to the collected workflow files, not a full-repository security approval. I am not repeating mooki's findings on the older revision as if they were still open. One small result-contract follow-up for the pilot, separate from the workflow corrections: keep **orchestration outcome** and **functional verdict** explicitly separate. I also replayed the current Comment script with a recording `gh` double and `OUTCOME=success`: - `results.txt` containing `SYNTHETIC required check scene_complete: FAIL` produces the `**success**` header and preserves the FAIL text below it. - No `results.txt` produces the same `**success**` header followed by `The runner wrote no results.txt.` That is consistent with the documented job-outcome reporting; I am **not** calling it a demonstrated false hardware PASS or a lost failure. It does mean that a consumer must not turn the bold job status into a test verdict. For a later machine-readable receipt, a conservative rule is: validate repository/PR/exact SHA/hardware/scene identity and schema first; take required check names from a separate trusted plan; a known workload failure, fatal/device-loss observation or required FAIL dominates incomplete evidence; missing/SKIPPED/UNKNOWN checks stay INCONCLUSIVE; PASS requires every named required check PASS and complete, clean process evidence. That is still an **untrusted self-report**, not authentication or isolation. The report should never execute receipt content. No runner registration, workflow dispatch, real game data, sandbox qualification or GPU result is implied by these replays. This is a COMMENT-level verification/follow-up, not a general security approval. — Nitnek, with AI-assisted analysis <!-- github-sync:r:5878837942 -->
github-sync commented 2026-11-12 17:59:31 +00:00
Author
Owner

@Nitnek commented on GitHub · 2026-11-12 17:56 UTC · original

CI follow-up for head 31e67594f173b1f20403342af3eb6587625b00c2: the two red source-check jobs need to be separated from the 31 passing hardware-workflow fixture tests.

I read the actual job logs, not just check_run.head_sha. Both checked out merge d7427b5b818a781a4f6de40d6fb7485a15e97878, merging this PR into bd9b07e6bdd4155c50aeb360c96bc65be19a63be. The merge's parents were verified. That base is the base of this run, not a claim about today's latest main.

For tests/test_readme_menu_rows.py, I exported that exact merge and its actual base, read the pure textual test, then ran it under Linux on both:

python3 -I -B tests/test_readme_menu_rows.py
merge d7427b5b: 5 tests, 2 failures, exit 1
base  bd9b07e6: 5 tests, 2 failures, exit 1

Both fail because Skip opening is a menu row missing from the readme option table (missing-row and order assertions). The test, readme.md and gpu/vk/vk_menu.cpp are byte-identical between that base and merge. This establishes that this particular CI failure is already present in the run's base, rather than introduced by these hardware workflow changes. It also explains why looking up this test at the bare PR head alone returned 404 while CI ran it from the merge.

The Linux job has a separate failure in test_nightly_two_runs_at_once_end_at_the_newer_commit (AssertionError: 1 != 0, 33 tests / 1 failure). I have not replayed or fixed that race in this follow-up, so I am not attributing it from the log alone. The same named flaky test is already discussed in #2626 and in the validation limits of #2779; those release changes address different problems and should not be conflated.

Logs inspected:

This was source/log analysis and a game-free Linux text-fixture replay only, not a Windows rerun, full suite, workflow dispatch, sandbox or GPU qualification. No blanket green-CI or approval claim.

AI-assisted analysis: Hermes Agent (OpenAI GPT-6.1-sol).

> [@Nitnek](https://github.com/Nitnek) commented on GitHub · 2026-11-12 17:56 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2807#issuecomment-6831608643) CI follow-up for head `31e67594f173b1f20403342af3eb6587625b00c2`: the two red source-check jobs need to be separated from the 31 passing hardware-workflow fixture tests. I read the actual job logs, not just `check_run.head_sha`. Both checked out merge **`d7427b5b818a781a4f6de40d6fb7485a15e97878`**, merging this PR into **`bd9b07e6bdd4155c50aeb360c96bc65be19a63be`**. The merge's parents were verified. That base is the base of this run, not a claim about today's latest main. For `tests/test_readme_menu_rows.py`, I exported that exact merge and its actual base, read the pure textual test, then ran it under Linux on both: ```text python3 -I -B tests/test_readme_menu_rows.py merge d7427b5b: 5 tests, 2 failures, exit 1 base bd9b07e6: 5 tests, 2 failures, exit 1 ``` Both fail because **`Skip opening`** is a menu row missing from the readme option table (missing-row and order assertions). The test, `readme.md` and `gpu/vk/vk_menu.cpp` are byte-identical between that base and merge. This establishes that this particular CI failure is already present in the run's base, rather than introduced by these hardware workflow changes. It also explains why looking up this test at the bare PR head alone returned 404 while CI ran it from the merge. The Linux job has a separate failure in `test_nightly_two_runs_at_once_end_at_the_newer_commit` (`AssertionError: 1 != 0`, 33 tests / 1 failure). I have **not** replayed or fixed that race in this follow-up, so I am not attributing it from the log alone. The same named flaky test is already discussed in #2626 and in the validation limits of #2779; those release changes address different problems and should not be conflated. Logs inspected: - https://github.com/NotRexed/Hookshot/actions/runs/43264986826/job/131346033132 - https://github.com/NotRexed/Hookshot/actions/runs/43264986826/job/131346033067 This was source/log analysis and a game-free Linux text-fixture replay only, not a Windows rerun, full suite, workflow dispatch, sandbox or GPU qualification. No blanket green-CI or approval claim. AI-assisted analysis: Hermes Agent (OpenAI GPT-6.1-sol). <!-- github-sync:c:6831608643 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2807
No description provided.