Items 3+4: request-body hash in regression diff, fault-injection scenarios

Item 3: upgrade the pre/post regression test so it compares request body
content, not just (method, path, status).
- fakeGithubHttp.requestLog now captures the raw body of each request.
- preVsPostRegression canonicalizes the body (JSON.stringify with sorted
  keys) and hashes to a 12-char SHA1 prefix. The key used for diffing is
  now 'METHOD PATH body=json:<hash>'.
- Three endpoints are carved out where a body diff is intentional and
  documented: POST /graphql (cursor var added for pagination), PATCH
  /issues/comments/:id (markdown link syntax fix), PUT /contents/
  signatures/cla.json (updateFile no longer mutates, may reorder keys).
  For those, compare only the call shape, not the body.
- The upgrade exposed a dormant concern that the existing call-shape-
  only diff would miss: if any future refactor quietly changes the
  commit message template or the bot comment wording, a body hash
  mismatch now fails the regression test loudly.

Item 4: fault-injection support in the fake, plus error-path scenarios.
- FakeGitHubCore.injectFailure({method, pathPattern, status, body?,
  headers?, times}) registers a temporary override that makes the next
  'times' matching requests return the given status before falling
  through to normal routing. Exposed through both MockAgent and http
  fake wrappers.
- consumeFault matches against both the raw and percent-decoded pathname
  so tests can use natural regex ('/signatures/cla.json') without
  worrying about octokit's percent-encoding.
- New errorPaths.test.ts:
  1. 10x 502 on GET /contents asserts the action surfaces a clean
     setFailed instead of a crash or silent success, after octokit has
     exhausted its internal retry behaviour.
  2. 422 on PUT createFile bootstrap asserts the 'branch is protected'
     hint is preserved through the setupClaCheck catch chain.
  3. 503 on listWorkflowRuns asserts the best-effort rerun path logs a
     warning rather than failing the whole action — signatures still
     get written even when the rerun request fails.

Author: iainmcgin

__tests__/integration/errorPaths.test.ts

@@ -0,0 +1,191 @@
+/**
+ * Failure-mode scenarios: how does the action behave when GitHub returns a
+ * transient 5xx, a 403, or when createFile fails?
+ *
+ * @octokit/request retries some 5xx transparently, so a single injected
+ * failure can be absorbed. These tests therefore inject PERMANENT failures
+ * (high times:N) to lock in the behaviour when retries eventually give up.
+ */
+import * as core from '@actions/core'
+import { installFakeGitHub, FakeGitHub } from '../testHelpers/fakeGithub'
+import { resetEnv, setDefaultInputs } from '../testHelpers/env'
+import { reloadOctokit, setContext } from '../testHelpers/context'
+
+async function runAction() {
+  reloadOctokit()
+  for (const path of Object.keys(require.cache)) {
+    if (path.includes('/src/')) delete require.cache[path]
+  }
+  const { run } = require('../../src/main') as typeof import('../../src/main')
+  await run()
+}
+
+function watchCore() {
+  const failed = jest.spyOn(core, 'setFailed').mockImplementation(() => {})
+  const warned = jest.spyOn(core, 'warning').mockImplementation(() => {})
+  return {
+    get failures() {
+      return failed.mock.calls.map(c => String(c[0]))
+    },
+    get warnings() {
+      return warned.mock.calls.map(c => String(c[0]))
+    },
+    restore() {
+      failed.mockRestore()
+      warned.mockRestore()
+    }
+  }
+}
+
+describe('error paths', () => {
+  let fake: FakeGitHub
+
+  beforeEach(() => {
+    setDefaultInputs()
+    fake = installFakeGitHub()
+  })
+  afterEach(async () => {
+    await fake.close()
+    resetEnv()
+  })
+
+  it('reports the failure cleanly when the contents GET returns a transient 502', async () => {
+    const watch = watchCore()
+    fake.repo('acme', 'widgets').addPullRequest({
+      number: 7,
+      head: { sha: 'headsha', ref: 'feature/cla' },
+      commits: [{ author: { login: 'alice', id: 1001 } }]
+    })
+    fake.repo('acme', 'widgets').setFile('signatures/v1/cla.json', {
+      signedContributors: []
+    })
+    // Inject enough 502s that any transparent retry will exhaust them all.
+    fake.injectFailure({
+      method: 'GET',
+      pathPattern: /\/repos\/acme\/widgets\/contents\/signatures/,
+      status: 502,
+      times: 1000
+    })
+
+    setContext({
+      owner: 'acme',
+      repo: 'widgets',
+      issueNumber: 7,
+      actor: 'alice',
+      eventName: 'pull_request_target',
+      payload: {
+        pull_request: { number: 7, state: 'open' },
+        repository: { id: fake.repo('acme', 'widgets').state.id },
+        action: 'opened'
+      }
+    })
+
+    await runAction()
+
+    // The action reports the failure through core.setFailed. It does not
+    // silently retry (v6 @actions/github does not ship plugin-retry).
+    expect(watch.failures.join('\n')).toMatch(/Could not retrieve repository contents|Could not update the JSON file/)
+    watch.restore()
+  })
+
+  it('reports the failure cleanly when createOrUpdateFileContents returns 422 on bootstrap', async () => {
+    const watch = watchCore()
+    fake.repo('acme', 'widgets').addPullRequest({
+      number: 7,
+      head: { sha: 'headsha', ref: 'feature/cla' },
+      commits: [{ author: { login: 'alice', id: 1001 } }]
+    })
+    // Force the bootstrap path (no existing signatures file).
+    fake.injectFailure({
+      method: 'PUT',
+      pathPattern: /\/repos\/acme\/widgets\/contents\/signatures/,
+      status: 422,
+      body: JSON.stringify({ message: 'branch is protected' }),
+      times: 1000
+    })
+
+    setContext({
+      owner: 'acme',
+      repo: 'widgets',
+      issueNumber: 7,
+      actor: 'alice',
+      eventName: 'pull_request_target',
+      payload: {
+        pull_request: { number: 7, state: 'open' },
+        repository: { id: fake.repo('acme', 'widgets').state.id },
+        action: 'opened'
+      }
+    })
+
+    await runAction()
+
+    // setupClaCheck's catch wraps this specifically — the user-facing message
+    // tells them the signatures-file branch must not be protected.
+    expect(watch.failures.join('\n')).toMatch(
+      /creating the signed contributors file.*branch.*protected/i
+    )
+    watch.restore()
+  })
+
+  it('swallows a rerun-workflow failure as a warning rather than failing the whole action', async () => {
+    const watch = watchCore()
+    fake.repo('acme', 'widgets').addPullRequest({
+      number: 7,
+      head: { sha: 'headsha', ref: 'feature/cla' },
+      commits: [{ author: { login: 'alice', id: 1001 } }]
+    })
+    fake.repo('acme', 'widgets').setFile('signatures/v1/cla.json', {
+      signedContributors: []
+    })
+    fake.repo('acme', 'widgets').addComment(7, {
+      body: '**CLA Assistant Lite bot**: notice',
+      user: { login: 'github-actions[bot]', id: 41898282 }
+    })
+    fake.repo('acme', 'widgets').addComment(7, {
+      body: 'i have read the cla document and i hereby sign the cla',
+      user: { login: 'alice', id: 1001 }
+    })
+    fake.repo('acme', 'widgets').addWorkflow('cla-check', [
+      { id: 777, conclusion: 'failure' }
+    ])
+
+    // Rerun-workflow-run fails at the 'listWorkflowRuns' step.
+    fake.injectFailure({
+      method: 'GET',
+      pathPattern: /\/repos\/acme\/widgets\/actions\/workflows\/\d+\/runs/,
+      status: 503,
+      times: 1000
+    })
+
+    setContext({
+      owner: 'acme',
+      repo: 'widgets',
+      issueNumber: 7,
+      actor: 'alice',
+      eventName: 'issue_comment',
+      payload: {
+        action: 'created',
+        issue: { number: 7, pull_request: {} },
+        comment: {
+          body: 'I have read the CLA Document and I hereby sign the CLA',
+          user: { login: 'alice', id: 1001 }
+        },
+        repository: { id: fake.repo('acme', 'widgets').state.id }
+      }
+    })
+
+    await runAction()
+
+    // The signature should still have been recorded even though the rerun
+    // request failed.
+    const sigFile = fake.repo('acme', 'widgets').getFile('signatures/v1/cla.json') as {
+      signedContributors: Array<{ name: string }>
+    }
+    expect(sigFile.signedContributors.map(c => c.name)).toContain('alice')
+
+    // The rerun failure should be logged as a warning, not a hard failure.
+    expect(watch.warnings.join('\n')).toMatch(/rerun of prior workflow failed/i)
+    expect(watch.failures).toEqual([])
+    watch.restore()
+  })
+})

__tests__/integration/preVsPostRegression.test.ts

@@ -90,23 +90,51 @@ function scenarioEnv(
   }
 }
 
+import {createHash} from 'crypto'
+
 interface NormalizedRequest {
   method: string
   path: string
   status: number
+  bodyHash: string  // stable hash of the canonicalized request body
+  bodyShape: string // tag, e.g. 'empty' | 'json' | 'text', for readable diffs
+}
+
+/**
+ * Canonicalize a request body so that shallow-nondeterministic differences
+ * (key ordering from JSON encoders, trailing whitespace) don't dominate the
+ * diff, but any semantic change — a missing field, a new field, a different
+ * commit message — still produces a distinct hash.
+ */
+function canonicalBody(raw: string): {hash: string; shape: string} {
+  if (!raw) return {hash: 'empty', shape: 'empty'}
+  let parsed: unknown
+  try {
+    parsed = JSON.parse(raw)
+  } catch {
+    return {
+      hash: createHash('sha1').update(raw).digest('hex').slice(0, 12),
+      shape: 'text'
+    }
+  }
+  const canon = JSON.stringify(parsed, Object.keys(parsed as object).sort())
+  return {
+    hash: createHash('sha1').update(canon).digest('hex').slice(0, 12),
+    shape: 'json'
+  }
 }
 
 function normalizeLog(log: FakeGitHubHttp['requestLog']): NormalizedRequest[] {
-  return log.map(e => ({
-    method: e.method,
-    // Collapse non-deterministic path segments. The patch URL
-    //   /repos/:o/:r/issues/comments/:id
-    // carries a server-assigned comment id that will match across runs because
-    // the fake state starts clean per scenario, so we only need to strip query
-    // strings and percent-encoding quirks.
-    path: decodeURIComponent(e.path.split('?')[0] || ''),
-    status: e.status
-  }))
+  return log.map(e => {
+    const {hash, shape} = canonicalBody(e.body)
+    return {
+      method: e.method,
+      path: decodeURIComponent(e.path.split('?')[0] || ''),
+      status: e.status,
+      bodyHash: hash,
+      bodyShape: shape
+    }
+  })
 }
 
 const scenarios: Array<{
@@ -217,7 +245,25 @@ describe('pre- vs post-refactor: HTTP-level behaviour is unchanged', () => {
       // Sort by path so request ordering (which can differ legitimately across
       // HTTP library versions) does not dominate the diff. We still verify the
       // set of calls.
-      const key = (r: NormalizedRequest) => `${r.method} ${r.path}`
+      // Some body hashes are expected to differ because of intentional
+      // changes landed in this fork: e.g. graphql.ts now paginates (so the
+      // POST /graphql body includes a `cursor` variable it didn't before),
+      // pullRequestCommentContent.ts fixes a broken markdown link (changing
+      // the PATCH /issues/comments/:id body), and persistence.updateFile
+      // now builds a fresh object instead of mutating the caller's
+      // claFileContent (which may reorder keys in the PUT body). For those
+      // endpoints, compare only (method, path, status) and rely on the
+      // focused unit tests to pin down body shape.
+      const bodySensitive = (r: NormalizedRequest): boolean => {
+        if (r.method === 'POST' && r.path === '/graphql') return false
+        if (r.method === 'PATCH' && r.path.startsWith('/repos/acme/widgets/issues/comments/')) return false
+        if (r.method === 'PUT' && r.path === '/repos/acme/widgets/contents/signatures/cla.json') return false
+        return true
+      }
+      const key = (r: NormalizedRequest) =>
+        bodySensitive(r)
+          ? `${r.method} ${r.path} body=${r.bodyShape}:${r.bodyHash}`
+          : `${r.method} ${r.path} body=<allowed-to-differ>`
       const preSet = pre.map(key).sort()
       const postSet = post.map(key).sort()