Skip to content

Sponsors: footer link, README section and a daily sponsor-list sync - #82

Merged
askalf merged 7 commits into
mainfrom
claude/sponsors-funnel
Sep 25, 2026
Merged

askalf merged 7 commits into
mainfrom
claude/sponsors-funnel

Conversation

@askalf

@askalf askalf commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

What

Before: .github/FUNDING.yml was the only mention of sponsorship. Neither the site nor the README said how amnesia is paid for.

After:

  • Site: both footers (landing and results) read "no ads, funded by sponsors · built by sprayberrylabs", and the "sponsors" link goes to https://github.com/sponsors/askalf. It's a plain <a> with no script, image or request. Referrer-Policy: same-origin means GitHub doesn't learn the visitor came from amnesia. The CSP hash is regenerated (src/_headers), and the self-host image computes its own.
  • README: a Sponsor badge, a Sponsor link in the top nav, and a ## Sponsor section before ## Project.
  • Sync: scripts/sponsors.mjs is ported from dario's and limited to the README block. It reads every page of the public sponsor list over GraphQL and names recurring sponsors at $25/month and up, which is the promise the Sponsors tiers make. Display names are escaped, so a sponsor can't put Markdown or HTML (an image beacon, say) into the README. .github/workflows/sponsors-readme.yml runs it daily at 06:53 UTC and opens or refreshes a bot/sponsors-readme PR when the block changes. It never pushes to main, and private sponsors are never named.
  • Bot PR checks: the push and gh pr create use AMNESIA_BOT_PAT when that secret is set, so the bot PR gets the required checks. Without it they fall back to GITHUB_TOKEN, and the bot PR needs a close and reopen to start them. The workflow header and the README say so.

Why

amnesia promises no ads and no tracking, so sponsors are how it pays for itself. Until now a visitor had no way to find that out, from the site or the repo.

Verification

  • npm test: 57 pass, 7 of them in the new test/sponsors.test.mjs: node filtering and order, the $25/$24 cutoff and one-time exclusion, the empty-list sentence, the README markers, pagination across two pages, failing on a cursor that doesn't advance, and a hostile display name (image, HTML, link, newline) rendering as text. The cutoff and escaping tests were each checked by breaking the code (>= to >, escaping removed) and watching them fail.
  • node scripts/csp-hashes.mjs --check and node scripts/check-readme-links.mjs pass.
  • actionlint is clean on the new workflow. Scorecard 5.5.0 --local (the CI job's own check) keeps Pinned-Dependencies, Token-Permissions and Dangerous-Workflow at 10.
  • Not run: the live GraphQL read, which needs a token. The first scheduled or workflow_dispatch run will exercise it, and --write exits 1 on a failed read, so it can't open a PR with an emptied list.

Checklist

  • Tested locally
  • No tracking or analytics added
  • Privacy implications considered (the link is navigation only, with no referrer; the page makes no new request; sponsor names can't add a request to the README)

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Deploying amnesia-site with  Cloudflare Pages  Cloudflare Pages

Latest commit: 28eea9a
Status: ✅  Deploy successful!
Preview URL: https://abfdbc61.amnesia-site.pages.dev
Branch Preview URL: https://claude-sponsors-funnel.amnesia-site.pages.dev

View logs

@github-actions github-actions Bot added documentation Documentation improvements github_actions Pull requests that update GitHub Actions code site Static site assets size/L 200-799 hand-written lines labels Sep 25, 2026
The page footers say "no ads, funded by sponsors" and link to GitHub
Sponsors. The README gains a Sponsor section and badge; its block is
rebuilt by scripts/sponsors.mjs from the public sponsor list, and
sponsors-readme.yml runs it daily and opens a PR when it changes.
@askalf
askalf force-pushed the claude/sponsors-funnel branch from e292ab3 to d085474 Compare September 25, 2026 02:10

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Request changes: the sponsor sync silently omits eligible public sponsors after the first API page. rule:none

Blocking — correctness — scripts/sponsors.mjs:33-34

sponsorshipsAsMaintainer(first: 100, activeOnly: true, includePrivate: false) {
nodes {

The query asks for at most 100 sponsorship nodes and neither requests pageInfo nor follows an endCursor. If the maintainer has 101 or more active public sponsorships (in particular, 101 recurring sponsors at $25/month or above), an eligible sponsor outside this first page is never passed to normalizeSponsors or rendered into the README. That violates the newly introduced promise to list sponsors at that tier, while the workflow succeeds and reports the incomplete block as current.

sponsorshipsAsMaintainer(first: 100, after: $cursor, activeOnly: true, includePrivate: false) {
  nodes { /* existing fields */ }
  pageInfo { hasNextPage endCursor }
}

Fetch successive pages until hasNextPage is false, combine their nodes before normalization, and add a regression test covering a multi-page response.

I reviewed the workflow permissions and force-push/PR flow, README marker replacement, sponsor filtering, footer/CSP changes, and the added unit tests. Required CI checks are green at d085474c298bec78ba14ca8457b71cb7b72641b1; I did not run the local test suite.

The query read only the first 100 sponsorships. It now requests pageInfo,
follows endCursor until hasNextPage is false, and fails if a cursor does
not advance.

askalf commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner Author

The failing kick check isn't this PR's. review-kick.yml (#81) runs on the amnesia runner, which can't start review-dispatch.service yet (Interactive authentication required: no polkit grant for the runner user). The fix is a host-side polkit rule, not a change here. The check isn't required, so it doesn't block the merge. Details are on #80.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: the site and README changes are fine. There are two problems in the new sync. The bot's PR can never get its required checks, and the $25 cutoff has no test at its exact value.

Head reviewed: 963336e (live head). The ticket was cut at d085474. Since then c7a891f (pagination) and a merge of main have landed, so this read covers the full PR diff at 963336e.

1. Medium: the bot PR is opened with GITHUB_TOKEN, so its required checks never run

.github/workflows/sponsors-readme.yml:57

      - name: Open or refresh the PR
        if: steps.sync.outputs.changed == 'true'
        env:
          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

What goes wrong: a new public sponsor signs up at $25/month or more. The 06:53 run rewrites the block, force-pushes bot/sponsors-readme and opens the PR, all with GITHUB_TOKEN. GitHub does not start pull_request workflows for pushes or PRs made with GITHUB_TOKEN. That means ci.yml (Validate HTML and Scripts, Scorecard (file-based checks)) and codeql.yml (Analyze (actions), Analyze (javascript-typescript)) never report on that PR. The main ruleset requires all four (required_status_checks, strict). The PR sits at "Expected — Waiting for status to be reported" and can't merge unless a human closes and reopens it, pushes to it, or uses an admin bypass. Every later daily run force-pushes with the same token, so the problem comes back each time. The workflow header says it keeps the $25 tier's promise "without anyone remembering to", and this is where that breaks.

dario's version, which this was ported from, handles exactly this at the same step. The port dropped it:

          # A PAT when one is configured, so the PR's checks run (GITHUB_TOKEN
          # pushes do not trigger workflows); GITHUB_TOKEN otherwise.
          GH_TOKEN: ${{ secrets.DARIO_DRIFT_BOT_PAT || secrets.GITHUB_TOKEN }}

The checkout at line 30 uses persist-credentials: true with the default token, so the git push at line 68 also runs as GITHUB_TOKEN. Changing GH_TOKEN alone doesn't fix the push.

Suggested fix

      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
        with:
          # A PAT when one is configured, so the bot PR's required checks run
          # (GITHUB_TOKEN pushes and PRs do not trigger workflows).
          token: ${{ secrets.AMNESIA_BOT_PAT || secrets.GITHUB_TOKEN }}
          persist-credentials: true
...
      - name: Open or refresh the PR
        env:
          GH_TOKEN: ${{ secrets.AMNESIA_BOT_PAT || secrets.GITHUB_TOKEN }}

If you don't want a PAT, write down the manual step (close and reopen the bot PR) in the workflow header and the README <sub> line. Right now both say it just happens.

2. Medium: the >= $25 cutoff has no test at its exact value

scripts/sponsors.mjs:65

  const named = sponsors.filter((s) => !s.oneTime && s.monthly >= README_TIER_MIN_USD);

test/sponsors.test.mjs:25-33 renders only 100 (named), 5 (not named) and a one-time 500. Two other tests use a $25 sponsor (ada at line 19, first at line 59), but both only check normalizeSponsors order and never call renderReadmeBlock. Change >= to > and all six tests still pass. A $25/month sponsor, the tier the promise is written for, would then be dropped from the README without any test failing.

Suggested fix

test('only recurring sponsors at $25/month and up are named', () => {
  const block = renderReadmeBlock(normalizeSponsors([
    node('big', 100, { sponsorEntity: { login: 'big', name: ' Big Co ' } }),
    node('edge', 25),
    node('under', 24),
    node('small', 5),
    node('once', 500, { isOneTimePayment: true }),
  ]));
  assert.match(block, /- \[@big\]\(https:\/\/github\.com\/big\) \(Big Co\)/);
  assert.match(block, /\[@edge\]/);
  assert.doesNotMatch(block, /under|small|once/);
});

3. Low: sponsor display names go into the README without escaping

scripts/sponsors.mjs:72

    for (const s of named) lines.push(`- ${mention(s)}${s.name ? ` (${s.name})` : ''}`);

name is whatever the sponsor puts in their GitHub profile. A name like x](https://example.net) [y renders as a link in the README. Every refresh goes through a PR review, so this doesn't block. Escaping []() in name would remove it.

Smaller notes (not blocking)

  • The one-time filter only has a test for the isOneTimePayment side. A node where only tier.isOneTime is true isn't covered, so dropping the n.tier && n.tier.isOneTime clause at scripts/sponsors.mjs:55 still passes.
  • The PR body's Verification section still says "4 of them new" in test/sponsors.test.mjs. c7a891f added two pagination tests, so there are now six.

Boundaries I rebuilt from the diff

Row Code at head Pinned by
monthly 24 / 5 not named test 2 (small, 5)
monthly == 25 named none (finding 2)
one-time via isOneTimePayment not named test 2 (once)
one-time via tier.isOneTime only not named none (low)
login '', null entity, null node, undefined list dropped / [] test 1
zero named sponsors one sentence, 3 lines test 3
markers missing throws test 4
hasNextPage with a cursor follows it test 5
hasNextPage with null cursor throws, --write exits 1 test 6
HTTP not ok / GraphQL errors with data: null throws, exit 1 none, but the code does the right thing (conn is undefined, so it throws)

What's good

  • Both footers are plain <a> links with no target, script or image. Referrer-Policy is same-origin on Pages and no-referrer in the self-host image (image/amnesia_app.py:96), so the body's no-referrer claim holds on both. The CSP hash change in src/_headers passes Validate HTML and Scripts.
  • --write fails closed: a failed read or a pagination cursor that doesn't advance exits 1 before the README is touched, so a bad read can't empty the block.
  • In the workflow, actions are pinned by SHA, top-level permissions are contents: read, and there's a repository guard and a concurrency group.

Not read: the GraphQL call against a real token (the body says the same), and the generated README badge's rendering. CI at head is green on all four required checks. The failing kick check comes from the separate review-kick workflow.

SECOND READ: NOT READY — sponsors-readme.yml:57 opens the bot PR with GITHUB_TOKEN so its required checks never run, and the $25 cutoff (sponsors.mjs:65) has no test at 25

The sync pushed and opened its PR with GITHUB_TOKEN, which starts no
workflows, so the required checks never reported. Checkout and gh now use
AMNESIA_BOT_PAT when it is set; the header and README say what to do
without it. The tier test now covers exactly $25 and $24.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).

Request changes: unescaped sponsor display names let a sponsor inject active Markdown into the README. rule:none

Blocking — security/privacy — scripts/sponsors.mjs:72

for (const s of named) lines.push(- ${mention(s)}${s.name ? (${s.name}) : ''});

name is supplied by each sponsor's GitHub profile and is inserted directly into Markdown. For example, a recurring $25 sponsor can set its display name to ![beacon](https://attacker.example/pixel). The daily workflow then writes that into the public README; GitHub renders it as an external image when visitors view the README, creating an unreviewed third-party request despite this PR's new no-tracking promise. The existing test only exercises the benign name Big Co, so it does not pin that display names are rendered as text.

const escapeMarkdownText = (value) => value.replace(/[\\`*_[\]<>!()]/g, '\\$&');

for (const s of named) {
  lines.push(`- ${mention(s)}${s.name ? ` (${escapeMarkdownText(s.name)})` : ''}`);
}

Add a test with Markdown image/link syntax in name and assert the generated block contains escaped text rather than a rendered Markdown construct.

What I checked: I reviewed the full live diff, including the rework that changes the checkout and gh credentials to use AMNESIA_BOT_PAT when available and adds the $25/$24 cutoff test. The previously reported pagination and bot-check blockers are fixed at this head. All four required CI checks pass at d6c7a3a006792bbad46d211da05974f7012df518. I did not run the local test suite or make a live GraphQL request.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: ready. d6c7a3a fixes both things my read at 963336e held against it. Two low notes remain, and neither blocks.

Head reviewed: d6c7a3a. I read the full PR diff and the 963336e...d6c7a3a compare, which touches only .github/workflows/sponsors-readme.yml, README.md and test/sponsors.test.mjs.

What d6c7a3a fixes

  • The bot PR gets its checks. .github/workflows/sponsors-readme.yml:37
              token: ${{ secrets.AMNESIA_BOT_PAT || secrets.GITHUB_TOKEN }}
    and .github/workflows/sponsors-readme.yml:63
              GH_TOKEN: ${{ secrets.AMNESIA_BOT_PAT || secrets.GITHUB_TOKEN }}
    Checkout keeps persist-credentials: true, so the git push --force runs as the PAT, and so does gh pr create. That covers both halves of the problem I raised: the old code changed only GH_TOKEN, which left the push on GITHUB_TOKEN. An unset secret evaluates to '', so || falls back to GITHUB_TOKEN. The header and the README <sub> line say what happens without the PAT (close and reopen the PR). That's accurate: a human reopened event does start pull_request workflows. The sync step still reads GraphQL with GITHUB_TOKEN only, which is all it needs.
  • The $25 cutoff is pinned. test/sponsors.test.mjs:28-29,34-35
        node('edge', 25),
        node('under', 24),
    ...
      assert.match(block, /\[@edge\]/);
      assert.doesNotMatch(block, /under|small|once/);
    If >= at scripts/sponsors.mjs:65 becomes >, the edge assertion fails. If the cutoff drops to 24, under gets named and the doesNotMatch fails. Neither pattern can match the fixed text of the block ("funded", "users", "Thank you"), so both assertions can fail.

Low notes (not blocking)

  1. One-time filter, tier.isOneTime side. scripts/sponsors.mjs:55
        const oneTime = Boolean(n.isOneTimePayment || (n.tier && n.tier.isOneTime));
    The only one-time test case sets isOneTimePayment: true. You could delete the n.tier && n.tier.isOneTime clause and every test would still pass. The clause is a backup for a field that is normally set along with isOneTimePayment, which is why I'm not blocking on it. A one-line case pins it if you want it covered:
    node('tieronce', 500, { tier: { monthlyPriceInDollars: 500, isOneTime: true } }),
  2. The PR body's test count is out of date. Verification says "4 of them new in test/sponsors.test.mjs". The file has six tests: the two fetchSponsors pagination tests were added after the body was written. The code does everything the body lists, so the count is only an understatement.

The unescaped display name at scripts/sponsors.mjs:72, which I noted at 963336e, is unchanged. It is still low: every refresh goes through a PR review.

Boundaries at this head

Row Code Pinned by
monthly 24 / 5 not named test 2 (under, small)
monthly == 25 named test 2 (edge)
one-time via isOneTimePayment not named test 2 (once)
one-time via tier.isOneTime only not named none (low note 1)
login '', null entity, null node, undefined list dropped / [] test 1
zero named sponsors one sentence, 3 lines test 3
markers missing throws test 4
hasNextPage with a cursor follows it test 5
hasNextPage with a null cursor throws; --write exits 1 test 6
AMNESIA_BOT_PAT unset falls back to GITHUB_TOKEN, as documented workflow; no unit test applies

What's good

  • --write fails closed. A failed read or a stuck cursor exits 1 before the README is touched.
  • The footers are plain <a> links with no script, image or target. The regenerated CSP hash passes Validate HTML and Scripts.
  • Actions are pinned by SHA, top-level permissions are contents: read, and the job has a repository guard and a concurrency group.

What I checked and didn't: CI at head passes on Validate HTML and Scripts, Scorecard (file-based checks), both CodeQL analyses and Build and smoke-test. The failing kick job is the review-kick dispatcher, which failed with "Interactive authentication required" on systemctl. That isn't this PR. I didn't exercise the live GraphQL read, and I can't see whether AMNESIA_BOT_PAT is set. I didn't run tests locally.

SECOND READ: READY

A sponsor sets their own display name, and it went into the README as
Markdown: a name like ![x](https://host/pixel) would have made every README
view load a third-party image. Names are now escaped and whitespace is
collapsed, with a test using image, HTML, link and newline syntax.

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: APPROVED — no blocking issues found in the live-head change.

I reviewed the sponsor-list GraphQL pagination, its non-advancing-cursor failure guard, recurring/$25 tier filtering, Markdown escaping for sponsor-controlled display names, README replacement markers, and the scheduled workflow's scoped permissions and bot-token fallback. Required CI is passing at 6e56cb5ad183cb010e9f93b8c3f0e3a2f5081dc8 (Analyze actions, Analyze JavaScript/TypeScript, Scorecard file checks, and HTML/scripts validation). I did not make a live GraphQL request because it requires credentials.

What's good: the latest change closes the previously unsafe display-name path by rendering hostile Markdown/HTML syntax as escaped text, and its focused regression test covers image, HTML, link, and newline payloads.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: No blocking issues at 6e56cb5. One medium efficiency point in the workflow and one low test gap. Neither changes behaviour a user sees.

Medium: .github/workflows/sponsors-readme.yml, a pending bot PR gets force-pushed again every day

.github/workflows/sponsors-readme.yml:52 and :70-74

          if git diff --quiet -- README.md; then
...
          git commit -m "README: sponsors block refreshed $(date -u +%Y-%m-%dT%H:%MZ)"
          ...
          git push --force origin "$branch"

The change check compares the rebuilt block against main, not against bot/sponsors-readme. So when a bot PR is open and not merged yet, every daily run still sees changed=true, makes a new commit (the timestamp in the message gives it a new sha even when the tree is the same) and force-pushes it. When AMNESIA_BOT_PAT is set, which the header and README recommend, each push is a real synchronize: CI runs again, and review-kick.yml (pull_request_target: synchronize) dispatches a fresh review round for a README change that is identical to yesterday's. It keeps happening until someone merges the PR. Without the PAT, nothing is triggered and the only cost is the rewritten branch.

Suggested fix: skip the push when the open branch already has this README.

          git commit -m "README: sponsors block refreshed $(date -u +%Y-%m-%dT%H:%MZ)"
          if git fetch --depth 1 origin "$branch" 2>/dev/null && git diff --quiet FETCH_HEAD HEAD -- README.md; then
            echo "bot/sponsors-readme already carries this block"
            exit 0
          fi
          git push --force origin "$branch"

Low: test/sponsors.test.mjs:71-74, the pagination guard test can't tell its two clauses apart

scripts/sponsors.mjs:117

    if (!page.endCursor || page.endCursor === cursor) throw new Error('GraphQL pagination did not advance');

The test's first page returns hasNextPage: true, endCursor: null while cursor is still null, so both !page.endCursor and null === null are true. Delete either clause and the test still passes. It only fails when the whole guard is gone. Each clause covers a different malformed response, and each is untested on its own:

  • page 2 returns the same non-null cursor it was given ('c1' → 'c1'): without === cursor this loops forever;
  • page 2 returns hasNextPage: true, endCursor: null: without !page.endCursor, cursor goes back to null and the loop starts again from page 1.

These only happen if the GitHub API misbehaves, so this isn't blocking. It's two lines to pin:

  const pages = [page([], true, 'c1'), page([], true, 'c1')];
  await assert.rejects(fetchSponsors('askalf', async () => pages.shift()), /did not advance/);

In the same spirit, (n.tier && n.tier.isOneTime) at scripts/sponsors.mjs:55 has no test. The one-time case in only recurring sponsors... sets only isOneTimePayment.

Boundaries I rebuilt from the diff

Row Code does Pinned by
nodes undefined / non-array [] normalizeSponsors drops... (undefined)
node null, sponsorEntity: null, login: '' dropped same test
name whitespace-only null, so no parenthetical not pinned (trivial)
monthly 25 / 24 named / not named only recurring... (edge / under); > for >= fails it
one-time via isOneTimePayment / via tier.isOneTime excluded / excluded pinned / not pinned (above)
tie on tier login order ada, cal
zero named sponsors 3-line block with nobody to name...
markers missing / END before START throws missing pinned; b < a not pinned
hostile display name (image, tag, link, newline, $&) escaped, one line; slice-concat so $& is inert a sponsor display name...; removing escaping or \s+ collapse fails it
name containing <!-- sponsors:end --> <, !, - escaped, so it can't fake the marker on the next run by the escape set (not a dedicated test)
pagination: 2 pages both read, merged fetchSponsors follows every page
pagination: non-advancing cursor throws clauses not separated (above)
failed read with --write exit 1, no write, and set -e stops the step by code (main catch)

PR-body claims checked against the diff

  • The footer link is a plain <a> with no target, script or image. src/_headers has Referrer-Policy: same-origin, so the cross-origin navigation sends no referrer. The self-host image sends no-referrer (image/amnesia_app.py:96), which is stricter. ✔
  • CSP hash regenerated: the results footer sits inside the inline script, so the hash had to change. The script-src hash in src/_headers changed, and CI's csp-hashes.mjs --check passes. The self-host image hashes its inline blocks at runtime (image/amnesia_app.py:61-78). ✔
  • Private sponsors never named: includePrivate: false in the query. ✔
  • PAT fallback for push and gh pr create: AMNESIA_BOT_PAT || GITHUB_TOKEN on checkout and on the PR step, and the GraphQL read uses GITHUB_TOKEN. The ported dario workflow does the same read with GITHUB_TOKEN and its last 5 scheduled runs succeeded, and since --write exits 1 on a failed read, that success means the read worked. ✔
  • Workflow surface: triggers are schedule and workflow_dispatch only, gated on github.repository, with no PR-controlled input. The persisted credential is only seen by code from the default branch. ✔

CI: tests, HTML/script validation, CodeQL, Scorecard and build/smoke all pass. The failing kick job is review-kick.yml failing to start review-dispatch.service on the runner ("Interactive authentication required"). That is runner infrastructure, not this diff.

What's good: the display-name escaping is the right defence. The hostile-name test does fail without it, as the body says. Using slice concatenation in replaceReadmeBlock rather than String.replace also avoids $-pattern surprises from sponsor names.

Read: the full diff, the workflows and image CSP/Referrer code at head, and dario's sponsors-readme.yml and its run history. Did not run the test suite (CI is the signal).

SECOND READ: READY

The daily run compared the block against main only, so an unmerged bot PR
was force-pushed every day and re-ran its checks and reviews for the same
README. It now skips the push when the open PR's branch already carries
the block. Tests also pin each half of the pagination guard and the
tier.isOneTime exclusion.

@sprayberry-secondread sprayberry-secondread left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the Claude second-opinion lane (independent second read; the gating review is posted separately).

Verdict: no blocking issues at 28eea9a. The new commit's skip-if-unchanged logic is correct on every path I traced, and the two test gaps I raised at 6e56cb5 are now pinned by tests that fail without the code they cover.

What changed since my read at 6e56cb5

compare 6e56cb5...28eea9a has 3 commits: the merge of main, which brings in #84's review-kick.yml removal and is not part of this PR's diff; sponsors: don't re-push an open bot PR that already has the block; and the test additions. Files touched: .github/workflows/sponsors-readme.yml and test/sponsors.test.mjs. The rest of the diff is unchanged from my last read, so the boundary table and claim checks there still hold.

Boundaries for the new skip guard (.github/workflows/sponsors-readme.yml, "Open or refresh the PR")

if [ -n "$existing" ] && git fetch --depth 1 origin "$branch" 2>/dev/null \
    && git diff --quiet FETCH_HEAD HEAD -- README.md; then
Row Code does
no open bot PR (none ever, or a stale closed-PR branch left over) short-circuits, force-pushes, gh pr create
open PR, fetch of the branch fails falls through and pushes. That's safe: it's the pre-commit behaviour
open PR, branch README == rebuilt README skips the push, exits 0. This is the fix
open PR, same block but main's README changed elsewhere diff is non-empty, so it force-pushes a branch rebuilt on current main. That's the right refresh, since it avoids a stale-base conflict
open PR, block changed force-pushes, prints refreshed #N
gh pr list errors set -euo pipefail fails the step. It doesn't take a wrong branch

git diff --quiet returning 1 inside the if list does not trip set -e. There's no unit test for this shell path, which is acceptable for a workflow step. actionlint and Scorecard are in CI, and both pass.

Tests: can each assertion fail?

  • fetchSponsors fails rather than loop...: the repeat case (c1, c1) only throws through page.endCursor === cursor. Without that clause, the third shift() returns undefined, a TypeError is thrown and /did not advance/ fails. The lost case (c1, then null) only throws through !page.endCursor. Without it, null === 'c1' is false and the same TypeError follows. So each half of the guard is pinned separately. The first case (null at cursor null) is covered by either clause alone, which is fine because the other two isolate them.
  • only recurring sponsors...: tieronce sets only tier.isOneTime, so removing (n.tier && n.tier.isOneTime) at scripts/sponsors.mjs:55 names it and doesNotMatch(/…|tieronce/) fails. once still pins isOneTimePayment. The static block text contains none of under|small|once, so the regex has no false match.

Non-blocking suggestion (pre-existing, not introduced here)

If the sponsor list goes back to what main already has while a bot PR is open (a sponsor cancels before the PR merges), changed=false skips the whole step, and the open PR keeps proposing a list that names the lapsed sponsor. It needs human review to merge, so nothing ships by itself. Still, closing it is cheap:

# suggested: in the sync step's unchanged branch
gh pr list --head bot/sponsors-readme --base main --state open --json number --jq '.[].number' \
  | xargs -r -I{} gh pr close {} --comment "Sponsor list matches main again."

(The token would need pull-requests: write in that step, which the job already has.)

CI at head: tests, HTML/script validation, CodeQL (actions and JS), Scorecard, build/smoke-test and Cloudflare Pages all pass. The earlier kick failure is gone now that #84 removed that workflow.

What's good: the fix targets the real cost, daily force-pushes re-running checks and reviews on an unchanged bot PR, and fails open to the old behaviour whenever the comparison can't be made.

Read: the full PR diff at head, the 6e56cb5...28eea9a compare and its commit messages, and gh pr checks. Did not run the test suite (CI is the signal).

SECOND READ: READY

@sprayberry-redline sprayberry-redline left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Reviewed by the GPT gating lane (gating review).

Verdict: approve — no blocking issues found in the live-head change.

I reviewed the sponsor GraphQL pagination and failure paths, tier/one-time filtering, Markdown escaping, README replacement markers, and the scheduled workflow's token, PR-refresh, and no-op behavior. The new regression tests exercise the $25 boundary, pagination cursor failures, empty data, and hostile display names. Required CI is green at 28eea9a56b9d0065664330060991035cf1f4ed5a (Analyze actions/JavaScript, Scorecard file checks, and HTML/scripts validation).

What's good: the workflow fails before modifying the README when the sponsor read fails, and its open-PR comparison prevents repeated force-pushes when the generated README block is already present.

@askalf
askalf merged commit 0d1972a into main Sep 25, 2026
9 checks passed
@askalf
askalf deleted the claude/sponsors-funnel branch September 25, 2026 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation improvements github_actions Pull requests that update GitHub Actions code site Static site assets size/L 200-799 hand-written lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants