From 497dc477588d0b8cf574ed40bb449a5506219033 Mon Sep 17 00:00:00 2001 From: Rhea Rafferty Date: Sun, 9 Aug 2026 10:34:46 +0000 Subject: [PATCH 1/3] Fix main: invert the reporter-session assertions, and trigger on the file they read main went red on 076cbf3. `production_config.test.ts` asserted that the deploy workflow wires the reporter-session keyring through the secret store, and the cleanup that merged in 076cbf3 removed exactly that wiring. The test was describing a state we deliberately left behind. Inverted rather than deleted. The assertions are the record of what was removed, and a deleted test cannot notice the wiring coming back: HANDS_FEEDBACK_REPORTER_SESSION_KEYS must not appear wrangler secret put FEEDBACK_REPORTER_... must not appear vars.HANDS_FEEDBACK_REPORTER_SESSION_ must not appear Validate reporter-session rollout inputs must not appear A suite of `doesNotMatch` passes on an empty string, on a renamed file, and on a read that quietly returned nothing, so it keeps a positive control anchoring it to a workflow that was actually read. The environment binding moves to its own test, because it outlived the case it was living in: the environment is named for the retired feature but is not part of it, and its branch policy is what confines this deploy to main. It is now matched at job-key indentation instead of requiring `jobs:\n deploy:\n environment:` to be adjacent. That adjacency is what broke here - a comment was added above the line explaining why it must never be deleted, and the test failed because of the explanation. A test that goes red when someone documents the thing it protects gets "fixed" by deleting the documentation. Both new cases were mutation-checked: removing the environment binding reds the binding test, re-adding a keyring reference reds the absence test, and restoring the file greens both. Why nobody saw it before the merge: the pull_request paths list here enumerated the files this job *reads in order to run*, which is not the same set as the files its tests *make assertions about*. production_config.test.ts opens deploy-hands-server.yml, that path matched nothing in the list, and a PR touching only it therefore ran no tests at all - six green checks, none of them this suite. The push-to-main trigger has no paths filter, by an earlier deliberate decision, and that is what caught it, one merge too late. The deploy workflow is now in the list with the distinction written down. "No test ran" and "every test passed" are the same green on a pull request page. Co-Authored-By: Claude Opus 5 --- .github/workflows/worker-tests.yml | 10 +++++ worker/test/production_config.test.ts | 58 +++++++++++++++++---------- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/.github/workflows/worker-tests.yml b/.github/workflows/worker-tests.yml index 3a7b87f..cb997f5 100644 --- a/.github/workflows/worker-tests.yml +++ b/.github/workflows/worker-tests.yml @@ -29,6 +29,16 @@ on: - "pnpm-lock.yaml" - "pnpm-workspace.yaml" - "tsconfig*.json" + # Files the suite *asserts on*, which is a different set from the files the job + # reads to run, and the one this list originally missed. production_config.test.ts + # opens the deploy workflow and makes claims about it, so a change to that file can + # invalidate a test here — and a PR touching only it matched nothing above, so the + # job never ran. That happened: the reporter-session cleanup deleted the wiring + # this suite asserted, all checks on the PR were green because the suite was not + # among them, and main went red on the merge commit instead. + # + # "No test ran" and "every test passed" are the same green on a pull request page. + - ".github/workflows/deploy-hands-server.yml" # No paths filter here, deliberately. Two lists are two things to keep in step, and # the first was already out of step: this file itself was missing from the push # list, so a change to the workflow could merge without main ever running it. The diff --git a/worker/test/production_config.test.ts b/worker/test/production_config.test.ts index b376d71..948c962 100644 --- a/worker/test/production_config.test.ts +++ b/worker/test/production_config.test.ts @@ -115,27 +115,41 @@ test("production config requires a closed key version before enabling reporter s } }); -test("deploy workflow binds the keyring only through the secret store", () => { +// This test used to assert that the reporter-session keyring reached the Worker only +// through the secret store. That keyring is gone - deleted from the GitHub environment, +// from the Worker, and finally from this workflow - so every "it is wired this way" +// assertion here now describes a thing that should not exist. Inverted rather than +// deleted: the assertions are the record of what was removed, and a deleted test cannot +// notice the wiring coming back. +test("the reporter-session keyring is gone from the deploy workflow", () => { const workflow = readFileSync(resolve(repositoryDir, ".github/workflows/deploy-hands-server.yml"), "utf8"); - assert.match(workflow, /jobs:\n deploy:\n environment: reporter-session-production\n/); - assert.match( - workflow, - /FEEDBACK_REPORTER_SESSION_KEYS: \$\{\{ secrets\.HANDS_FEEDBACK_REPORTER_SESSION_KEYS \}\}/, - ); - assert.match( - workflow, - /wrangler secret put FEEDBACK_REPORTER_SESSION_KEYS --config/, - ); - assert.doesNotMatch(workflow, /FEEDBACK_REPORTER_SESSION_KEYS: \$\{\{ vars\./); - assert.match( - workflow, - /HANDS_FEEDBACK_REPORTER_SESSION_ENABLED: \$\{\{ vars\.HANDS_FEEDBACK_REPORTER_SESSION_ENABLED \}\}/, - ); - assert.match( - workflow, - /HANDS_FLAGSHIP_APP_ID: \$\{\{ vars\.HANDS_FLAGSHIP_APP_ID \}\}/, - ); - const validationIndex = workflow.indexOf("- name: Validate reporter-session rollout inputs"); - const deployIndex = workflow.indexOf("- name: Deploy Hands Worker and admin assets"); - assert.ok(validationIndex >= 0 && deployIndex > validationIndex, "enabled inputs fail before Worker deploy"); + + assert.doesNotMatch(workflow, /HANDS_FEEDBACK_REPORTER_SESSION_KEYS/); + assert.doesNotMatch(workflow, /wrangler secret put FEEDBACK_REPORTER_SESSION_KEYS/); + assert.doesNotMatch(workflow, /vars\.HANDS_FEEDBACK_REPORTER_SESSION_/); + assert.doesNotMatch(workflow, /- name: Validate reporter-session rollout inputs/); + + // Positive control. Every assertion above passes on an empty string, on a file that + // was renamed out from under this test, and on a read that silently returned nothing - + // "the wiring is absent" and "there is no file here" are the same result. This anchors + // them to a workflow that was actually read. + assert.match(workflow, /HANDS_FLAGSHIP_APP_ID: \$\{\{ vars\.HANDS_FLAGSHIP_APP_ID \}\}/); +}); + +// Split from the case above because it outlived it. The environment is named for the +// retired feature but is not part of it: its deployment branch policy is what confines +// this production deploy to main, and the cleanup that removed the keyring came within a +// commit of removing this too. +// +// Matched on the line at job-key indentation rather than on `jobs:\n deploy:\n +// environment:` as before. That earlier form required the two lines to be adjacent, so +// it broke the moment a comment was added above `environment:` to explain why it must +// stay - a test that fails when someone documents the thing it protects will be +// "corrected" by deleting the documentation. Anchoring to indentation keeps the property +// (a job-level binding, not a string appearing somewhere in the file) without pinning +// what may sit between them. +test("the deploy job stays bound to its environment", () => { + const workflow = readFileSync(resolve(repositoryDir, ".github/workflows/deploy-hands-server.yml"), "utf8"); + assert.match(workflow, /^ deploy:$/m); + assert.match(workflow, /^ environment: reporter-session-production$/m); }); From dc607f584b140938cd0c1400b213d5a6a4312ecb Mon Sep 17 00:00:00 2001 From: Rhea Rafferty Date: Sun, 9 Aug 2026 10:38:04 +0000 Subject: [PATCH 2/3] Widen the trigger to .github/workflows/** instead of naming the one file Deriving 'which files do these tests assert on' is itself a list that can miss one, and it misses in the direction of green - the same failure this entry exists to fix, one level up. A wildcard has nothing to miss. Cost measured rather than assumed: the last eight runs of this job took 28-42s, median 35. Thirty-five seconds on an unrelated workflow edit is affordable waste; a guard that silently does not run is an unobservable failure. `paths` is a performance optimisation and was being used as a correctness boundary. Co-Authored-By: Claude Opus 5 --- .github/workflows/worker-tests.yml | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/.github/workflows/worker-tests.yml b/.github/workflows/worker-tests.yml index cb997f5..0c5170d 100644 --- a/.github/workflows/worker-tests.yml +++ b/.github/workflows/worker-tests.yml @@ -29,16 +29,26 @@ on: - "pnpm-lock.yaml" - "pnpm-workspace.yaml" - "tsconfig*.json" - # Files the suite *asserts on*, which is a different set from the files the job - # reads to run, and the one this list originally missed. production_config.test.ts - # opens the deploy workflow and makes claims about it, so a change to that file can - # invalidate a test here — and a PR touching only it matched nothing above, so the - # job never ran. That happened: the reporter-session cleanup deleted the wiring - # this suite asserted, all checks on the PR were green because the suite was not - # among them, and main went red on the merge commit instead. + # Files the suite *asserts on* — a different set from the files the job reads in + # order to run, and the one this list originally missed. production_config.test.ts + # opens the deploy workflow and makes claims about it; that path matched nothing + # above, so a PR touching only it ran no tests. That happened: the reporter-session + # cleanup deleted the wiring this suite asserted, every check on the PR was green + # because this suite was not among them, and main went red on the merge commit. + # + # The whole directory, not the one file that broke. Listing the asserted files + # means deriving that list, and a derivation that misses one is silent in the + # direction of green — the same failure this entry exists to fix, one level up. + # A wildcard has nothing to miss. + # + # The cost of erring wide is measured, not assumed: the last eight runs of this + # job took 28-42s, median 35. Thirty-five seconds on an unrelated workflow edit is + # affordable waste; a guard that silently does not run is an unobservable failure. + # `paths` is a performance optimisation and was being used as a correctness + # boundary. # # "No test ran" and "every test passed" are the same green on a pull request page. - - ".github/workflows/deploy-hands-server.yml" + - ".github/workflows/**" # No paths filter here, deliberately. Two lists are two things to keep in step, and # the first was already out of step: this file itself was missing from the push # list, so a change to the workflow could merge without main ever running it. The From e4bf99cfa209ef8b8076b3808eb246b1c8bb4286 Mon Sep 17 00:00:00 2001 From: Rhea Rafferty Date: Sun, 9 Aug 2026 10:40:19 +0000 Subject: [PATCH 3/3] =?UTF-8?q?Add=20container/**=20=E2=80=94=20the=20same?= =?UTF-8?q?=20defect,=20found=20by=20census,=20on=20#97's=20own=20detector?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cors_allow_headers.test.ts scans container/src and container/package.json because the container has no cors() today and its own suite does not run here. Without container/** a PR touching only container/ does not run it - and a PR introducing the first cors() call is, almost by definition, a PR touching only container/. The instrument would have been absent from exactly the change it exists to catch, which is the trigger condition task #97 currently rests on. Its own `registrations.length > 0` guard is correctly written and would not have helped. A denominator only means anything in a run that happens. Second instance in one census, which is why these entries are wildcards rather than a list derived again. Co-Authored-By: Claude Opus 5 --- .github/workflows/worker-tests.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/worker-tests.yml b/.github/workflows/worker-tests.yml index 0c5170d..8ee2b73 100644 --- a/.github/workflows/worker-tests.yml +++ b/.github/workflows/worker-tests.yml @@ -49,6 +49,18 @@ on: # # "No test ran" and "every test passed" are the same green on a pull request page. - ".github/workflows/**" + # cors_allow_headers.test.ts scans container/src and container/package.json + # (SOURCE_DIRS, MANIFESTS) because the container has no cors() today and its own + # suite does not run here. Without this entry, a PR touching only container/ does + # not run it — and a PR introducing the first cors() call is, almost by definition, + # a PR touching only container/. The instrument would have been absent from exactly + # the change it exists to catch. Its own `registrations.length > 0` guard is + # correctly written and would not have helped: a denominator only means anything + # in a run that happens. + # + # Second instance of the same defect in one census, which is why the entries above + # are wildcards and why this list is not being derived again. + - "container/**" # No paths filter here, deliberately. Two lists are two things to keep in step, and # the first was already out of step: this file itself was missing from the push # list, so a change to the workflow could merge without main ever running it. The