Skip to content

Governance: a GitHub PR webhook receiver that verifies first and only ever wakes (#704 S8) [PROTECTED: owner merges] - #842

Merged
jayden1711 merged 3 commits into
mainfrom
issue-704-s8
Oct 20, 2026
Merged

jayden1711 merged 3 commits into
mainfrom
issue-704-s8

Conversation

@jayden1711

@jayden1711 jayden1711 commented Oct 19, 2026 •

Copy link
Copy Markdown
Owner

Intent: Slice S8 of governance mode (#704). It adds the GitHub PR webhook receiver as a standalone module: it verifies each delivery first and only ever wakes the gate. A delivery never carries anything the gate decides on. The gate always re-reads the PR, its checks and its reviews through the API. Nothing mounts the receiver yet: S9 wires it next to the dashboard's other verify-first routes, and polling stays the fallback. So no install changes behaviour. This is independent of S1 (#833) and doesn't need its token.

Risk class: protected. It adds src/governance-webhook.ts. src/** is engine (.worklane/review.yaml), so the owner merges.

Part of #704 (slice S8; it does not close the issue).

Round 2 (0aa88658, after round 1's CHANGES NEEDED). Main 71316ebd is merged in, not rebased.

  • No header is ever echoed.
  • A body resent as another event is refused.
  • The handler never rejects.
  • "Compromised component" is corrected.
  • The size is re-measured against 71316ebd with the UI built.

What it does

receiveDelivery({secret, repo, headers, body, window}) checks a delivery in this order. Nothing of a delivery is read before the step that needs it.

  1. No secret, or one shorter than 16 characters: 503 for every delivery (fail closed).

  2. A body over 1 MiB: 413, unread.

  3. X-Hub-Signature-256 must be sha256=<hex> of HMAC-SHA256 over the raw body, compared with timingSafeEqual. Otherwise 401, with the same body every time.

  4. Only now is X-GitHub-Event read:

    • ping gets 200;
    • pull_request, pull_request_review, check_run, check_suite and status go on;
    • anything else gets 400.
  5. X-GitHub-Delivery must be a GUID that hasn't been seen in the last hour. Otherwise it gets 400, or 200 with no wake for a repeat. The window is bounded, and an id goes into it only after its signature passes.

  6. The JSON is parsed. Round 2: the body must have its event's shape (GitHub's top-level keys), or it gets 400:

    • pull_request and pull_request_review need a pull_request object;
    • check_run and check_suite need their own object;
    • status needs sha, state and context strings and no PR or check object.

    A PR's and a review's bodies may pass as each other, since both name the same one PR.

  7. A payload that holds a Claude credential is refused (Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711, holdsClaudeCredential).

  8. repository.full_name must be the instance's repo, compared in any case. Otherwise it gets 202 with no wake.

What it wakes:

  • A PR or review event wakes its PR.
  • A check event wakes its own pull_requests list, with duplicates removed.
  • A status event, an empty list, or more than 20 PRs wakes the whole repo once (pr: null), so the gate lists open PRs again.

Every why is a fixed text (round 2). None of them echoes a header, the payload or a name.

handleGithubWebhook(req, res, ctx) does the same over HTTP:

  • It answers 405 to anything but POST.
  • It refuses a body by its content-length, or once more than 1 MiB has been read. It then answers 413 and drops the connection.
  • It hands each wake to ctx.wake.
  • Round 2: it never rejects. A client that drops the connection mid-body, or a wake or log that throws, ends with the request and response destroyed, and the promise resolves.

By-design protection

  • Opaque wake. A delivery's whole output is a PrWake {repo, pr}:
    • repo is the instance's configured repo, never the payload's spelling.
    • pr is a positive safe integer, or null.
    • It's frozen, and its constructor throws unless it gets a symbol the module doesn't export.
    • isWake accepts only objects in a module-private WeakSet.
  • No payload data leaves the module. Heads, checks, review states, labels, the sender and text never leave it: verify(), fits() and wakesOf() aren't exported, and the parsed payload is a local. So a signed delivery whose payload lies about a head, a check or a review can at most make the gate re-read one PR (measured: test "a wake carries the configured repo and a PR number only").
  • Unsigned headers can't widen a wake or reach the log (round 2). The event must fit the signed body's shape, so a captured PR body can't be resent as a repo-wide status or check wake (measured). Every refusal text is fixed, so no header text reaches ctx.log (measured).
  • Verify first, in code order. The signature check comes before the event, the delivery id or the JSON is touched (measured: a badly signed, unparseable body with a bogus event and delivery id gets 401, not 400).
  • The guard is built in. The Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711 guard is called on every verified payload, in both the pure function and the HTTP reader. The existing body-reader test (test/claude-credential-guard.test.ts:32) passes with no new exemption.
  • No lint is added.

What this can never do

  • Merge, hold, label or comment on anything. It has no GitHub client, and nothing imports it (inferred: new file; measured: grep -rn "governance-webhook.js" src/ finds no importer).
  • Make the gate decide differently. A wake has no field to decide on (measured: payloads that differ in head, merged state, labels, sender, review or comment give identical wakes, whose only keys are pr and repo). The S9 acceptance test that one PR gets the same decision by webhook and by polling needs the gate, so it lands with S9.
  • Store or log a secret or a credential. It holds only the delivery-id window, in memory. Since round 2, no text from a delivery, header or body, reaches why (measured: the round-1 leak, where the unsigned event header reached why uncapped, is fixed and tested).
  • Reject its promise (round 2, measured: test "a client that drops the connection mid-body").

Compromised component

  • Someone without the secret and without a captured delivery, on the network. Every delivery they forge gets 401 or 413 before any parse. Each costs the receiver one HMAC over at most 1 MiB. There's no refusal counter or rate limit in this slice; S9 mounts it behind one (inferred).
  • The holder of a captured delivery (round 2; round 1 missed this actor). That's anyone who saw one delivery in transit, at a TLS-terminating proxy, or in the App's delivery log. GitHub signs the body only, never X-GitHub-Event or X-GitHub-Delivery. They need no secret:
    • Replay under fresh GUIDs. One captured body replayed 50 times gave 50 wakes, all for that same PR (measured). The delivery-id window drops only GitHub's own redeliveries; it doesn't stop this. What bounds it is S9's rate limit and per-PR coalescing (inferred). The cost is extra API re-reads; a decision can't change (inferred from the wake's shape, which is measured).
    • Resend as another event. Round 1: a pull_request body sent as status or check_suite turned a one-PR wake into a repo-wide one (measured by the reviewer). Round 2: refused with 400 (measured).
    • Text in a header. Round 1: echoed into why. Round 2: fixed texts only (measured).
  • Someone holding the webhook secret (a leaked secret, or GitHub itself). They can sign any body, so they can wake any PR number, or the whole repo, as often as they like. Each wake costs the gate a re-read through the API; they can't change a decision (inferred from the wake's shape, which is measured). Rotating the App's secret ends it.
  • An outside agent or person who opens PRs. They control payload contents: PR body, title, labels and review text. None of it leaves the module. A token pasted into a PR body makes that delivery refused, and polling still covers that PR (measured: the Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711 test).
  • Agent user, coordinator, dashboard, other instances, hub viewer. Nothing reaches this module until S9 mounts it (inferred). An agent can propose edits to it, which wait for the owner as an engine change (inferred from review.yaml).
  • Someone with push access. Same as today: changing this file is an owner-merged engine change.

How it was verified

  • Fail-without, round 1 (measured). The module was new, so the main proof was that the tests fail on the tree without it. On unchanged 6e0d7d81:

    • npm run build: error TS2307: Cannot find module '../src/governance-webhook.js';
    • the test run: Error [ERR_MODULE_NOT_FOUND], # fail 1.

    The Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711 case was written before the guard and failed against the module's first build (not ok 9, 11 of 12 passing).

  • Fail-without, round 2 (measured). I wrote the three new tests first and ran them against the round-1 module (058de90d merged with 71316ebd, module unchanged): # tests 15, # pass 12, # fail 3.

    • not ok 10 no unsigned header is ever echoed.
    • not ok 11 the event header must fit the body's shape.
    • not ok 12 a client that drops the connection mid-body: actual 'rejected: aborted'.
  • After (measured). governance-webhook and claude-credential-guard together: # tests 18, # pass 18, # fail 0.

  • Neighbouring tests (measured). These scan all of src/: package-files, review, feature-map, brand, isolate, push-check, change-files-renames, owner-terminal-removed, slot-dir-writes and slot-path. Result: # tests 228, # pass 228, # fail 0. CI runs the full suite.

  • Other checks (measured).

    • npm run typecheck: no errors.
    • npm run check:denylist: denylist: 1311 files clean.
    • node scripts/check-dco.mjs origin/main HEAD: DCO: all commits signed off. That includes the merge commit, made with --signoff.
    • The push gate: 2 commit(s) and 1 tip(s) clean.

Adversarial pass

I ran these from a scratch script against the built dist/src/governance-webhook.js at 0aa88658, with real local HTTP servers on 127.0.0.1. No repo file was edited.

Attack Result (measured)
Two X-Hub-Signature-256 headers (node joins them) 401
A 15-character secret, correctly signed 503
__proto__ key smuggling repository 202, no wake
repository.full_name acme/app/ 202, no wake
Event header Pull_Request 400, fixed text
check_run naming PR "12" (string) and 12.0 only 12, the JSON number, wakes
check_suite naming 500 PRs one repo-wide wake
Replay with the GUID upper-cased 200, no wake ("seen")
20,000 distinct delivery ids window size 10,000 (bounded)
Signed body of exactly 1 MiB of spaces 400 (not JSON), no wake
Prototype pollution PrWake.prototype.pr = 99 the wake's own frozen pr (12) wins
HTTP, chunked 2 MB body with no content-length 413, no wake
HTTP, no secret configured 503
Round 2: one captured signed PR body replayed 50× with fresh GUIDs 50 wakes, all for PR 12. Named gap; S9's rate limit and coalescing bound it
Round 2: captured PR body resent as status / check_suite / check_run 400 "its body is not that event's", no wake. Round 1: a repo-wide wake
Round 2: event header holding a setup token 400, why = "not an event that wakes the gate". Round 1: the token was in why
Round 2: 8,000-character event header 400, fixed 32-character why. Round 1: 8,036 characters
Round 2: check_run entry whose base is another repo (#99) wakes PR 99 here. Named gap: one harmless re-read
Round 2: an evicted id replayed after 10,000 signed ids wakes again. Named gap: moot beside the fresh-GUID replay
Round 2: HTTP client aborts mid-body the handler resolves. Round 1: rejected aborted
Forged wakes (spread, by hand, JSON round trip, structuredClone, Proxy, Object.create, swapped prototype, new PrWake(Symbol())) none passes isWake; the constructor throws (test)
A forged delivery naming a real delivery id 401, and the real one still wakes later (test)
A signed payload with merge without review, an approved review, green checks, another head the same wake as a plain one (test)

What I fixed because of it.

Named gaps, for S9 (inferred):

  • Replay of a captured delivery under fresh GUIDs wakes the same PR again. The window stops only GitHub's own redeliveries. S9's rate limit and per-PR coalescing bound it.
  • check_run.pull_requests entries aren't checked against their base repo. A mismatched entry wakes that number in this repo, which costs one re-read. GitHub's entries carry base.repo.name, not the full name.
  • An evicted id can be replayed after 10,000 signed ids.
  • A refusal counter or rate limit for unsigned floods, and where the secret file is read from. The App setup already installs webhook-secret (src/setup/app-manifest.ts:515,633), but the manifest's webhook is inactive today.

On unlisted events. They get a 400. A GitHub App also receives installation events it didn't subscribe to, so those will show as failed deliveries in the App's log. That's cosmetic: nothing is lost, because nothing in them would wake the gate (inferred).

The shape rule follows GitHub's documented top-level keys for each event (inferred from GitHub's webhook docs, not re-fetched here). If GitHub ever sent a body this rule doesn't fit, that delivery is refused and polling covers it, so it fails closed.

Package size

Measured with npm pack --dry-run --json --ignore-scripts through the npm wrapper. The baseline is a separate worktree at main 71316ebd. Both trees ran npm run build and npm run build:web, so dist/web is included.

  • Tarball: 1,903,282 → 1,906,340 (+3,058).
  • Unpacked: 5,938,018 → 5,949,351 (+11,333).
  • Files: 512 → 513.
  • Per file: dist/src/governance-webhook.js +11,333 (new). That's the only file that changed; the test isn't packed.
  • Round 1's body said +3,222 (without the UI, against 6e0d7d81). The reviewer measured round 1's head with the UI against 71316ebd at +2,551; that's the correct round-1 figure.
  • Round 2 grows it by the shape check, the catch and the comments.
  • PACKAGE_BUDGET (1,950,000 / 6,060,000) is unchanged and still has headroom.

CHANGELOG

None. Nothing is mounted, so users see nothing (inferred). S9 adds the bullet when the endpoint exists.

New dependencies

None (node:crypto, node:http only).

… ever wakes

Governance mode (#704) needs GitHub's PR events to reach the gate faster than the 60 s poll, without letting a
delivery decide anything. This adds the receiver as a standalone module; nothing mounts it yet (slice S9), so no
install changes behaviour.

It follows the automations hooks gateway's verify-first order: no secret (or one under 16 characters) refuses
everything; a body over 1 MiB is refused unread; X-Hub-Signature-256 must be the HMAC-SHA256 of the raw body,
compared in constant time, before the event, the delivery id or the JSON is looked at. Then only pull_request,
pull_request_review, check_run, check_suite and status wake (ping answers; anything else is refused), the delivery
id must be a GUID not seen in the last hour (a bounded window, recorded only after the signature passed), and a
payload holding a Claude credential is refused (#711).

A delivery's whole result is a PrWake: the configured repo and a PR number, or null to list the repo's open PRs
again. It is frozen, made only by the receiver and recognised through a module-private WeakSet; no head, check,
review or text from the payload leaves the module, so the gate always decides on what the API says.

Test (fails without): test/governance-webhook.test.ts. On the tree without src/governance-webhook.ts the build
reports TS2307 and the run fails with ERR_MODULE_NOT_FOUND (the module is new); the #711 case was written before the
guard and failed against the first build of the module (11 of 12 passed).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (round 1/2)

Reviewed head 058de90d against #704 (slice S8: the verify-first PR webhook receiver) and main 71316ebd. The design is sound. The signature check really does come first, the wake is opaque and can't be forged, and CI is green. Three small things block a PASS: one leak path from an unsigned header into the log, and two claims that are false as written.

Required changes

  1. The unsigned X-GitHub-Event header reaches the log verbatim, past the Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711 guard and with no length bound.
    • The code. src/governance-webhook.ts:155 builds why as `${event || 'no event'} is not an event that wakes the gate`, and handleGithubWebhook hands why to ctx.log (:207).
    • Why the signature doesn't stop it. GitHub's HMAC covers only the body, never the headers. So anyone holding one captured signed body (no secret needed) can put any text there.
    • Measured (scratch probe against the built head):
      • An event header holding a setup token (sk-ant-oat01- + 40 characters) gives status 400 | why contains the token: true.
      • An 8,000-character event header gives a 8,036-character why.
    • Why it matters. The Auth docs: who can read agents' credentials and whether the engine stores a token match the code (coordinator sudo, dashboard token paste) #711 guard runs on the parsed payload only (:144, :191). The PR's "can never do: store a secret or a credential" stops being true once S9 writes why to a log (inferred).
    • Fix, by design. Make the refusal a fixed text that never echoes the header, e.g. 'not an event that wakes the gate'. Add a test that a signed delivery whose event header holds a setup token gives a why without it.
  2. Correct the false claim in "Compromised component" (principle 5).
    • The claim. "Someone without the secret, on the network. Every delivery they send gets 401 or 413 before any parse."
    • Why it's false. A captured signed delivery needs no secret. X-GitHub-Delivery and X-GitHub-Event aren't signed, so a fresh GUID gets past the replay window every time.
    • Measured:
      • One captured (body, signature) pair replayed 50 times with fresh GUIDs: wakes = 50.
      • The same pull_request body replayed with the event set to check_suite or status: 202 [["acme/app",null]], so a one-PR wake becomes a repo-wide one.
    • What to change.
      • Add a "holder of a captured delivery" actor: anyone who sees one delivery in transit, at a TLS-terminating proxy, or in the App's delivery log.
      • Say in the body and in the module comment (:16-17) that the window drops GitHub's own redeliveries, not deliberate replays.
      • Say that S9's rate limit and coalescing are what bound it.
      • Add both attacks to the adversarial table.
    • Impact. Low. It only wakes, and the gate re-reads through the API (inferred from the wake's shape, which is measured).
  3. "Always ends the request" (:197) is false when a client aborts mid-body.
    • Measured. A raw socket sent Content-Length: 1000 and 100 bytes, then closed. handleGithubWebhook rejected with ECONNRESET.
    • Why it matters. The PR's own test server calls it as void handleGithubWebhook(...) (test/governance-webhook.test.ts:165). Mounted that way, an unsigned client could cause an unhandled rejection, which ends a Node 22 process by default (inferred). dashboard.ts's top-level try/catch (:2832) would catch it if S9 awaits the call inside it (inferred). automations/webhook.ts readBody has the same pattern on main.
    • Fix. Either catch the read error inside handleGithubWebhook and destroy the request, or correct the doc comment and state the "await it inside a catch" contract for S9. A test with an aborting client is preferred.

Checks

# Check Result
1 Verify-first Met (measured).
Before the HMAC, only the secret length, body.length and the signature header are touched, and the header goes through an anchored 64-hex regex. The event, delivery id and JSON are read only after signed() passes (:149-157).
240,000 unsigned deliveries with fresh GUIDs left the window at size 0.
The body buffer (at most 1 MiB) is the only allocation that grows with input, and the HMAC needs it.
1a Constant-time compare Met.
Measured, median ns over 40,000 interleaved runs with a 2 KB body:
- wrong first byte: 2,400
- wrong last byte: 2,400
- all zeros: 2,400
- 62 or 66 hex digits: 200, because the regex rejects them before any HMAC
The early return leaks only whether the header is well-formed, which is public. Inferred: the regex pins 64 hex digits, so timingSafeEqual always compares 32 bytes with 32 and never throws.
1b Raw bytes Met (measured).
- A BOM signed with the BOM: 400 (not JSON), no wake.
- A BOM body signed without the BOM: 401.
- CRLF signed as CRLF: 202. CRLF signed as LF: 401.
- A latin1 re-encoding: 401.
- Over HTTP, a chunked body with a 4-byte UTF-8 character split across chunks: 202, 1 wake.
2 Replay window Gaps (measured).
- After 10,000 fresh signed ids, the oldest real id is evicted and its replay wakes again: 202 [["acme/app",7]].
- Recording only after verification works: 20,000 unsigned deliveries left the window at size 1, and the real id was still "seen".
- The bigger gap is required change 2, which makes the eviction moot.
3 Wake can't carry data or be forged Met (measured).
Forgery attempts:
- A subclass passing a look-alike Symbol('webhook wake mint') throws.
- No symbols are reachable on PrWake or its prototype.
- A wake's own keys are repo and pr.
Repo checks:
- Another repo's payload: 202, no wake.
- A payload about the fork repo itself: 202, no wake.
- full_name with a trailing space: no wake.
- A PR from a fork into this repo wakes PR 12 in the configured repo. That's correct: the gate re-reads it, and #704 says a fork PR is never governed.
Minor gap: a check_run whose pull_requests entry has another repo as its base still wakes that number here ([["acme/app",99]]). It's harmless, because the gate re-reads one PR.
4 #711 placement Met (inferred from code, measured by the PR's test).
The guard runs after the HMAC and the JSON parse. That's the right place, because running it earlier would mean parsing unauthenticated JSON. The order is named in the body's step 6 and in the module comment.
Not met for headers: see required change 1.
5 Durability rule Met. The body names three by-design protections: the opaque PrWake (symbol-gated, WeakSet, frozen), verify-first in code order, and the gate's re-read through the API ("Intent", "What this can never do"). No lint is added (measured: 2 files changed).
6 Fail-without Met (measured). Method (b): a fresh worktree at 71316ebd, with test/governance-webhook.test.ts written in by Write (cmp with the head's file: identical).
- npx tsc -p tsconfig.json: error TS2307: Cannot find module '../src/governance-webhook.js'.
- node --import ./dist/test/isolate.js --test dist/test/governance-webhook.test.js: Error [ERR_MODULE_NOT_FOUND] ... dist/src/governance-webhook.js, # fail 1.
A compile error is unavoidable here because the module is new.
On the head, governance-webhook + claude-credential-guard: # tests 15, # pass 15, # fail 0.
7 Package size Met (measured).
npm pack --dry-run --json --ignore-scripts through the wrapper, web UI built in both trees, base main 71316ebd:
- Tarball: 1,903,282 to 1,905,833 (+2,551).
- Unpacked: 5,938,018 to 5,947,332 (+9,314).
- Files: 512 to 513.
- The only changed file is dist/src/governance-webhook.js (added, 9,314).
Both sizes are under PACKAGE_BUDGET (1,950,000 / 6,060,000), so no raise is needed. The body's tarball figure (+3,222, without the UI, against 6e0d7d81) differs. Updating it is optional.
8 Typecheck, denylist, neighbours Met (measured).
- npm run typecheck: clean.
- npm run check:denylist: denylist: 1311 files clean.
- brand, feature-map, slot-dir-writes: # tests 62, # pass 62.
- Not run here: owner-terminal-removed (its PATH reaches /usr/bin) and package-files (I measured the pack directly). CI covers both.
9 Merge with main Met (measured): git merge-tree --write-tree origin/main HEAD gives a clean tree, 5c182e3e.

#704 acceptance criteria this slice touches

Criterion Status
A delivery with a bad signature is refused before anything else Met (measured: the PR's test 1, plus probe 1).
A signed delivery whose payload claims a different head, checks or reviews gives the same decision as the API's answer Partially met. Measured: the wakes are identical. The decision itself needs the gate (S9).
The same PR, decided by webhook and by polling, gets an identical decision Not in this slice. Deferred to S9, as the body says.
Polling stays the fallback Met (measured: nothing imports the module, by grep -rn governance-webhook src scripts package.json).

Protection

  • Marking. src/** is engine (.worklane/review.yaml), so this PR is protected. The marking is correct (measured from the PR's JSON): the title has [PROTECTED: owner merges], the body has **Risk class:** protected, and the PR has the needs-owner label.
  • "What this can never do". The no-GitHub-client and no-importer claims are true (measured by grep). "Never stores a credential" depends on required change 1.
  • "Compromised component". It misses the holder of a captured delivery (required change 2).
  • No regression for existing installs. Nothing is mounted, so no route an install had closed opens (inferred from the importer grep, which is measured).

Scope, hygiene, CI

Named gaps (non-blocking; for S9 or the owner)

  • Replay of a captured delivery. It needs no secret. It also covers an evicted id once 10,000 signed ids have arrived, and a body sent under another event name to widen a wake to the whole repo. Impact is limited to extra API re-reads (inferred). S9's rate limit and per-PR coalescing should bound it.
  • The event header isn't checked against the body's shape. Any signed body can be sent as status, which gives a repo-wide wake. This is the same root cause as the first gap.
  • check_run.pull_requests entries aren't checked against their base repo. A mismatched entry wakes that number in this repo, which costs one harmless re-read.
  • S9 must await handleGithubWebhook inside the dashboard's catch if required change 3 is fixed only in the doc.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…vent, never reject on a dropped client

Review round 1 on #842 found three things, all measured. GitHub signs the body only, so anyone holding one captured
delivery can set the headers: the unsigned X-GitHub-Event header was echoed into the log text (a setup token or 8,000
characters went through), and a captured pull_request body resent as status or a check event became a repo-wide wake.
And handleGithubWebhook rejected with "aborted" when a client dropped the connection mid-body, which an unawaited
caller would turn into an unhandled rejection.

Every refusal is now a fixed text that names no header. The body must have its event's shape (GitHub's top-level
keys), so a PR body sent as status, check_run or check_suite is refused; a PR's and a review's bodies may pass as
each other, since both name the same one PR. The handler catches any error, destroys the request and response, and
always resolves. The module comment now says the delivery-id window drops only GitHub's own redeliveries; a
deliberate replay under fresh ids only wakes the same PR again, bounded by S9's rate limit and coalescing.

Test (fails without): test/governance-webhook.test.ts. Against the round-1 module the three new tests fail
(header echo, body shape, and the dropped client: 'rejected: aborted'); 12 of 15 pass.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 2: 0aa88658

Main 71316ebd is merged in (not rebased; the merge commit is signed off). Each required change, with its evidence:

  1. No unsigned header is echoed.
    • The unlisted-event refusal is now the fixed text not an event that wakes the gate, and every other why is fixed too.
    • Test: "no unsigned header is ever echoed". It covers a setup token, an 8,000-character header and a newline-split header.
    • Measured: it failed on the round-1 module and passes now. In the probe, the 8,000-character header now gives a 32-character why.
  2. "Compromised component" is corrected.
    • The body adds "The holder of a captured delivery". It says the window drops only GitHub's own redeliveries, and that S9's rate limit and coalescing bound deliberate replays. The module comment says the same.
    • Both attacks are in the adversarial table (measured): 50 replays under fresh GUIDs gives 50 wakes, all for one PR.
    • The cheap tightening is done. The body must fit its event's shape, so a PR body resent as status, check_run or check_suite gets 400. It only refuses. A PR's and a review's bodies may pass as each other, since both name the same one PR.
    • Test: "the event header must fit the body's shape", a 5×5 matrix of bodies and events. Measured: it failed on the round-1 module and passes now.
  3. The handler never rejects. handleGithubWebhook catches any error, destroys the request and response, and resolves.
    • Test: "a client that drops the connection mid-body: the handler settles, never rejects". It uses a raw socket with content-length: 1000, 100 bytes, then destroy.
    • Measured: on the round-1 module it gave 'rejected: aborted'; now it gives resolved.
  4. Body.
    • Size, re-measured against 71316ebd with the UI built: main is 1,903,282, matching yours. This head adds +3,058 (unpacked +11,333). The body states your +2,551 as round 1's correct figure.
    • Named gaps: check_run.pull_requests isn't checked against the base repo, and an evicted id can be replayed after 10,000 signed ids. Both were measured again in my probe.

Fail-without (measured). The new tests against the round-1 module: # tests 15, # pass 12, # fail 3. At 0aa88658, the receiver and credential-guard tests pass 18 of 18. The neighbouring tests pass 228 of 228, typecheck is clean, and the denylist reports 1311 files clean.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: PASS

Round 2 at head 0aa88658. All three of round 1's required changes are fixed, each with a test that fails on the round-1 module. The new shape check refuses every event swap I tried. The handler settled at every stage I could abort or stall it.

Round-1 required changes

# Change Result
1 Unsigned header echoed into why Fixed (measured).
I sent 15 kinds of refusal, with the event header set to a setup token, to 8,000 characters, or to 8,000 characters plus a token. The delivery header was set to a token or 8,000 characters, and the signature header to a token.
Every why was one of 10 fixed texts, 4 to 35 characters long, and none held the token.
The only why built from input is ${v.event}: ${n} wake(s) (wakesOf). There, event has already passed the 5-entry allowlist (inferred from code).
2 "Compromised component" claim Fixed (read from the body).
The body adds "The holder of a captured delivery" and says the window drops only GitHub's own redeliveries. It names S9's rate limit and coalescing as what bounds a deliberate replay. The module comment says the same (src/governance-webhook.ts:16-23).
Measured: 50 captured replays under fresh GUIDs give 50 wakes, all for one PR. That's the documented gap.
3 handleGithubWebhook rejected on abort Fixed (measured). See the never-rejects table below. Across 13 requests: 0 pending, 0 rejected, 0 unhandledRejection or uncaughtException.

Shape matching (measured, receiveDelivery on the built head)

  • A captured pull_request body under each allowed event:
    • as pull_request or pull_request_review: 202, a wake for PR 7;
    • as check_run, check_suite or status: 400 "its body is not that event's".
  • Real GitHub body shapes of other events, sent under each of the 5 allowed events:
    • push, issue_comment, deployment_status, workflow_run, workflow_job, installation, ping: 400 under all 5.
    • pull_request_review_comment and pull_request_review_thread pass as pull_request or pull_request_review and wake the same PR (7). That's harmless: the same one PR.
    • A full check_run body (with a nested check_suite) wakes only as check_run.
  • Can a body match two shapes? The shapes are mutually exclusive, apart from the intended pull_request/pull_request_review pair.
    • A body with both a PR object and the status strings wakes only as a PR event.
    • A body with both a check_run and a check_suite object is refused under all 5.
    • {pull_request: null} or {pull_request: [1]} plus the status strings passes only as status. That needs a body GitHub signed, and no real event has that shape (inferred).
  • Every allowed event has a shape check: the switch in fits() covers all 5 WAKE_EVENTS, and TypeScript checks the return on every path (inferred from code, typecheck clean).
  • ping still works: a ping with a non-JSON body gets 200, no wake, and the fixed why "ping". Ping is answered before the parse.

Never rejects, at each stage (measured)

Real local server on 127.0.0.1 with an ephemeral port, the handler called with void, and process-level traps for unhandledRejection and uncaughtException. "Settled" is when the handler's promise resolved, timed from the request's start.

Case Client saw Handler
a. Headers only, then abort dropped resolved, 2 ms
b. 100 of 1,000 bytes, then abort dropped resolved, 1 ms
c. Chunked, half a chunk, then abort dropped resolved
d. Full body, then abort before the reply dropped resolved
e. Chunked 1.1 MiB, then abort dropped resolved
f. Slow client: body in 10-byte pieces, 100 ms apart 202 resolved after 804 ms
g. Never finishes: 10 of 1,000 bytes, then silence (server requestTimeout 1,500) 408 Request Timeout resolved once the server's timeout fired (after about 87 s, measured)
h. Chunked 2 MiB, no abort 413 resolved, 3 ms
i. content-length 2 MiB, 10 bytes sent 413 resolved, before the read
j–l. wake(), log() or secret() throws connection closed resolved
m. GET, then abort dropped resolved
  • The cap while reading holds (measured): case h answered 413 within 3 ms while the client was still sending 2 MiB.
  • How many bytes are read past the cap: at most 1 MiB plus one chunk. That's inferred from the per-chunk check at readDelivery, because the socket was already gone when I tried to count.

Fail-without (method b, measured)

  • Setup. A fresh worktree at the round-1 head 058de90d, with the round-2 test/governance-webhook.test.ts written in by Write. cmp with the head's file: identical.
  • Build and run. npx tsc -p tsconfig.json is clean. Then node --import ./dist/test/isolate.js --test dist/test/governance-webhook.test.js: # tests 15, # pass 12, # fail 3.
    • not ok 10 - no unsigned header is ever echoed: actual 'sk-ant-oat01-e3a1… is not an event that wakes the gate'.
    • not ok 11 - the event header must fit the body's shape: expected 400, actual 202.
    • not ok 12 - a client that drops the connection mid-body: actual 'rejected: aborted'.
  • On the head (0aa88658): governance-webhook + claude-credential-guard + brand gives # tests 20, # pass 20, # fail 0. The receiver and the guard account for 18 of those.

Other checks (measured)

  • Typecheck and denylist. npm run typecheck is clean. npm run check:denylist reports denylist: 1311 files clean.
  • Package size. npm pack --dry-run --json --ignore-scripts through the wrapper, with build and build:web run in both trees, against main 71316ebd:
    • Tarball: 1,903,282 to 1,906,340 (+3,058).
    • Unpacked: 5,938,018 to 5,949,351 (+11,333).
    • Files: 512 to 513.
    • The only changed file is dist/src/governance-webhook.js.
    • This matches the body exactly, and both sizes are under PACKAGE_BUDGET (1,950,000 / 6,060,000).
  • Merge with main. git merge-tree --write-tree origin/main HEAD gives a clean tree, 9dc76c2e. Against main the diff is still only the 2 new files. The CHANGELOG, package.json and lock changes between rounds come from merging in main.
  • CI. Every test shard on Node 22 and 24, dco, denylist, push-gate, scope, scan, new-deps, agentshield and desktop pass. Run 39334202930 has headSha 0aa88658 and success. The real-systemd jobs are skipped as usual.
  • Commits. All 3, including the merge commit, have Signed-off-by, and none has a Co-Authored-By or other attribution trailer.
  • Protected marking. The title has [PROTECTED: owner merges], the body has **Risk class:** protected, the needs-owner label is set, and both sections are present.
  • Body. Claims are labelled. The round-2 adversarial rows match what I measured. "Part of Governance mode: the engine runs only its evaluator, merge policy and locks in front of PRs other agents or people open, with no agents of its own; external reviews and CI are policy inputs, never the decision #704" is right for a slice.

Protection and durability

  • By-design protections (named in the body): the opaque PrWake, verify-first in code order, the re-read through the API, and now a shape check that only refuses. No lint is added.
  • "Can never do" claims. "Store or log a credential" and "reject its promise" are now true (measured above).
  • No regression for existing installs. Nothing imports the module, so no install's routes change (inferred).

Named gaps (non-blocking; for S9 or the owner)

  • Captured-delivery replay under fresh GUIDs wakes the same PR again (50/50, measured). It's documented in the body, and S9's rate limit and coalescing bound it.
  • An evicted id can be replayed after 10,000 signed ids. It's documented, and moot beside the fresh-GUID replay.
  • check_run.pull_requests entries aren't checked against their base repo. It's documented and costs one re-read.
  • New in round 2: the module has no read deadline of its own. A client that never finishes holds the handler and up to 1 MiB of buffer until the server's requestTimeout fires.
    • Measured: Node 22's default requestTimeout is 300,000 ms.
    • Measured: with it set to 1,500 ms, the stalled request was settled only when the server's periodic check ran, after about 87 s.
    • S9 should mount it on a server with a short request timeout, or behind the dashboard's existing limits (inferred).

@jayden1711
jayden1711 merged commit 71b2474 into main Oct 20, 2026
27 checks passed
@jayden1711
jayden1711 deleted the issue-704-s8 branch October 20, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant