Fix GreasyFork webhook sync: restore the release-asset URL form - #59
Fix GreasyFork webhook sync: restore the release-asset URL form#59nsheaps wants to merge 3 commits into
Conversation
…ires GreasyFork stopped picking up new versions after #40 switched every published script's @downloadURL/@updateURL (and, with them, the sync URL registered on GreasyFork) from https://github.com/<org>/<repo>/releases/latest/download/<pkg>.user.js to https://raw.githubusercontent.com/<org>/<repo>/latest/packages/<pkg>/dist/script.user.js #40's reasoning covered only half of GreasyFork's contract. It is right that `file_from_root_for_url` reduces the raw-blob URL to the correct git path (`packages/<pkg>/dist/script.user.js`) by discarding `latest` as the ref. But matching a script is a round trip, not a one-way parse: on a "release published" webhook, GreasyFork feeds that path back through `urls_for_ref` to REGENERATE candidate URLs - once with the release's own tag_name, once with the default branch - and then selects scripts with `sync_identifier IN (<those URLs>)`, byte equality. `latest` is neither `release/<timestamp>` nor `main`, so the stored URL was never among the regenerated candidates, every delivery fell through to "No scripts found.", and nothing synced. `releases/latest/download/` is the one base form `urls_for_ref` special-cases to emit with no ref segment appended, so it is the only URL that round-trips to itself and stays stable across releases. Restored, keeping #48's derive-from-the-package-directory improvement: - build-userscript.mjs derives @downloadURL/@updateURL as releases/latest/download/<pkg-dir>.user.js (byte-identical to the pre-#40 values, verified against them for all three packages). - release.yaml re-adds the flat repo-root <pkg>.user.js mirror in the tagged dist commit - `file_from_root_for_url` discards the packages/<pkg>/dist/ nesting, so that flat path is what GreasyFork reads with `git show <release-tag>:<path>`. - release.yaml re-adds the release-asset upload - GreasyFork reads content from git, but userscript managers that installed straight from GitHub poll @updateURL over HTTP, which only resolves with the asset attached. - The floating `latest` git tag stays put so scripts installed via the raw-blob URL during the #40 window keep updating. - Documented the whole matching contract at the top of release.yaml, in build-userscript.mjs, and in the README, since the trap is in the step that is easy to miss. Verified by porting GreasyFork's own `file_from_root_for_url` / `urls_for_ref` / `inject_script_info` logic and replaying both URL forms: the raw-blob-at-latest URL fails the match, the release-asset URL matches. NOTE: the sync URL is an account-side setting on GreasyFork, so each script's "sync from URL" must be set back to https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js for this to take effect.
Userscript Version PreviewPreview only — versions and CHANGELOGs are bumped automatically on merge to
|
… package The URL fix in the previous commit touches only scripts/build-userscript.mjs, and change detection scopes its diff to packages/<pkg>/. So the release job would have computed has_bumps=false on merge, skipped every one of its conditional steps, and shipped nothing - the corrected @downloadURL/@updateURL would never have been built, uploaded, or synced. Verified before the change: $ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview false | _no package changes detected_ | | | | scripts/build-userscript.mjs renders the `// ==UserScript==` metablock into every published artifact, so a change to it rewrites every artifact's bytes. Declare it as a shared build input: when it differs from the change base, every opted-in package counts as changed. Deliberately narrow - exactly the one file whose content lands inside every built script, not "any file outside packages/". Adding a shared input here is the mechanism for anything else that gets baked into every artifact later. After: $ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview true | github-actions-grafana-jump | 0.2.13 | 0.2.14 | will-bump | | github-to-graphite-button | 0.3.8 | 0.3.9 | will-bump | | graphite-to-github-button | 0.3.11 | 0.3.12 | will-bump | Per-package detection is unchanged when no shared input moved, checked against real history in a worktree at 215ef50 (a commit touching one package only): $ ./scripts/auto-bump-packages.sh --change-base=215ef50^ --version-base=215ef50^ --preview true | github-actions-grafana-jump | 0.2.12 | 0.2.13 | will-bump | and a no-op diff still reports nothing: $ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview # on main false | _no package changes detected_ | | | |
…n checks All four P2 nits from Henry's review on #59. 1. SHARED_BUILD_INPUTS entries are git pathspecs, and a pathspec matching nothing exits 0 with no output - verified: $ git diff --name-only HEAD~1..HEAD -- scripts/does-not-exist.mjs exit=0 So renaming a listed file would silently flip SHARED_CHANGED back to false and re-arm the exact ship-nothing failure the mechanism exists to prevent. Each entry is now verified to resolve at HEAD first, failing loudly instead. 2. Dropped -f from the root-copy `git add`. Confirmed only the nested path is ignored, so -f was misleading there: $ git check-ignore -v github-to-graphite-button.user.js # no match $ git check-ignore -v packages/.../dist/script.user.js .gitignore:147:dist/ packages/.../dist/script.user.js Kept -f on the nested add, with a comment on each saying which is which. 3. Removed the dead `dist-sha` step output (and the now-unused `id:` on that step) - nothing read `steps.dist-commit.outputs.dist-sha`; `Tag release` reaches the dist commit via bare HEAD. 4. Extracted the greasyforkPublish opt-in check into scripts/publishable-packages.sh. It was spelled out in three places - the two release.yaml steps plus auto-bump-packages.sh - so renaming the flag or adding a condition meant editing three places, with a half-applied change shipping silently. Now one place answers it. Output verified identical to the previous inline check. Both callers capture the helper's output into a variable rather than process-substituting it, so a failure in the helper aborts the step under `set -e` instead of feeding the loop an empty list - the same silent-degrade class as nit 1. Added an explicit empty-list guard for the same reason: a release run reaching those steps with nothing publishable is an error, not a no-op. Dropped the duplicated missing-artifact guard in the upload step per the review's alternative, since the earlier dist-commit step already fails on that condition and `cp` under `set -e` would too. Validation: # detection unchanged for this PR $ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview true | github-actions-grafana-jump | 0.2.13 | 0.2.14 | will-bump | | github-to-graphite-button | 0.3.8 | 0.3.9 | will-bump | | graphite-to-github-button | 0.3.11 | 0.3.12 | will-bump | # new guard fires on a stale entry (previously: silent false) $ sed -i 's|build-userscript.mjs|renamed-away.mjs|' scripts/auto-bump-packages.sh $ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview ::error::SHARED_BUILD_INPUTS entry 'scripts/renamed-away.mjs' does not exist at HEAD exit=1 # helper output identical to the old inline check $ diff /tmp/old-optin.txt /tmp/new-optin.txt # no differences # no over-triggering: package-only change at 215ef50 still bumps one package $ (cd <worktree@215ef50> && ./scripts/auto-bump-packages.sh \ --change-base=215ef50^ --version-base=215ef50^ --preview) | github-actions-grafana-jump | 0.2.12 | 0.2.13 | will-bump | # no-op diff on main still reports nothing $ (cd <worktree@main> && ./scripts/auto-bump-packages.sh \ --change-base=main --version-base=main --preview) false | _no package changes detected_ | | | | # both rewritten release loops dry-run correctly (git/gh echoed) $ <dist-commit + upload loops> git add -f packages/<pkg>/dist/script.user.js git add <pkg>.user.js x3 gh release upload <tag> <dir>/<pkg>.user.js --clobber x3 $ bash -n scripts/auto-bump-packages.sh scripts/publishable-packages.sh $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/release.yaml'))"
There was a problem hiding this comment.
### ✅ All four P2 nits from prior review addressed — approve
- ✅
SHARED_BUILD_INPUTS now guards each entry with git cat-file -e "HEAD:$input" — a rename fails loudly instead of silently reverting to SHARED_CHANGED=false.
- ✅
-f dropped from the root-copy git add, kept on the nested add with asymmetric comments explaining which is which.
- ✅ Dead
dist-sha output + unused id: dist-commit removed; push atomicity preserved (dist commit's objects still reach the remote via the tag refs).
- ✅ Opt-in check factored into new
scripts/publishable-packages.sh (100755) — replaces three inline copies (author caught one in auto-bump-packages.sh:85 that the prior review had missed).
- ✅ Bonus catch: both release loops now capture the helper's output into a variable rather than process-substituting it, so a helper failure aborts under
set -e instead of silently feeding an empty list — same silent-degrade class as nit 1, correctly generalised.
- ✅ Explicit empty-list guard added at both release call sites (
[ -z "$PUBLISHABLE" ]) — a release run with nothing publishable is treated as impossible-by-construction rather than a no-op.
Click to expand for full details
SHARED_BUILD_INPUTS now guards each entry with git cat-file -e "HEAD:$input" — a rename fails loudly instead of silently reverting to SHARED_CHANGED=false.-f dropped from the root-copy git add, kept on the nested add with asymmetric comments explaining which is which.dist-sha output + unused id: dist-commit removed; push atomicity preserved (dist commit's objects still reach the remote via the tag refs).scripts/publishable-packages.sh (100755) — replaces three inline copies (author caught one in auto-bump-packages.sh:85 that the prior review had missed).set -e instead of silently feeding an empty list — same silent-degrade class as nit 1, correctly generalised.[ -z "$PUBLISHABLE" ]) — a release run with nothing publishable is treated as impossible-by-construction rather than a no-op.How each prior P2 was addressed
Nit 1 — SHARED_BUILD_INPUTS silent-drop (scripts/auto-bump-packages.sh:83-88). Every listed pathspec is now verified to resolve at HEAD via git cat-file -e "HEAD:$input" before it is used as a diff pathspec. The ::error:: message names the file and points at the file to update, so the fix is discoverable. The comment above the loop explains the why — that pathspecs matching nothing exit 0 with no output, which is the ship-nothing failure class this whole mechanism exists to prevent. Author replayed the rename case (sed -i 's|build-userscript.mjs|renamed-away.mjs|' ...) and it errored loudly as intended.
Nit 2 — git add -f on the root copy (.github/workflows/release.yaml:262-270). Dropped -f on the root-level copy (git add "$root_copy"), kept it on the nested one (git add -f "$src"), and added a comment on each saying which is which. Cross-checked .gitignore at HEAD: line 147 is dist/, which covers packages/<pkg>/dist/script.user.js but not <pkg>.user.js at repo root. Author's tradeoff observation is fair — a future root-level *.user.js ignore rule would silently stop the mirror from being staged. That is a strictly better failure mode than the blanket -f hiding it just as silently, and the asymmetric comments make the assumption legible.
Nit 3 — dead dist-sha output (.github/workflows/release.yaml). Removed the echo "dist-sha=..." line and the now-unused id: dist-commit. Verified via grep -rn 'dist-sha' .github/ that no consumer remained (nor any if: expression that could have made it load-bearing without appearing in a ${{ steps... }} interpolation). Tag release reaches the dist commit via bare HEAD; the atomic push carries the dist commit's objects transitively via refs/tags/${RELEASE_TAG} and +refs/tags/latest. Nothing needed the sha to construct the push.
Nit 4 — opt-in / src-check duplication (scripts/publishable-packages.sh, new). Factored into a helper printing <pkg-dir-name> per line for every greasyforkPublish: true package. Author noticed a third copy in scripts/auto-bump-packages.sh:85 that the prior review had missed — that's the copy that decides whether a package is auto-bumped at all, so a partially-applied flag rename could have bumped a package and then not published it, or published one that was never bumped. Same silent-divergence class as the bug this PR fixes; the extra scope was warranted. Verified helper output (github-actions-grafana-jump, github-to-graphite-button, graphite-to-github-button) matches the three opted-in packages; the two _template/template packages are correctly excluded (both are "private": true with no greasyforkPublish).
Bonus: the process-substitution catch
The initial version of commit 3 used done < <(scripts/publishable-packages.sh) in the release loops. Process substitution discards the helper's exit code, so under set -euo pipefail a failing helper would have fed the loop an empty list and published nothing silently — the exact class of bug this PR exists to fix, just re-armed under a different trigger. Both callers now do PUBLISHABLE="$(scripts/publishable-packages.sh)" up front. In bash 5 (the GHA default), a non-zero exit from $() in a top-level assignment does trigger set -e, so the failure is caught. The explicit empty-list guard immediately below ([ -z "$PUBLISHABLE" ]) covers the impossible-by-construction case where the helper returns cleanly but with nothing.
Also correctly dropped the duplicated missing-artifact guard from the upload step: cp under set -e fails just as loudly, and the earlier Commit built userscripts for GreasyFork sync step provides the descriptive message on first-touch.
What was checked, and how
- Re-read
scripts/auto-bump-packages.sh,scripts/publishable-packages.sh, and.github/workflows/release.yamlat9d7aef6in full. - Verified
scripts/publishable-packages.shis100755via the git tree API. - Confirmed
.gitignoreline 147 covers onlydist/, not root-level<name>.user.js— the-f-drop analysis is correct at HEAD. - Confirmed only three packages have
"greasyforkPublish": true(github-actions-grafana-jump,github-to-graphite-button,graphite-to-github-button);_templateandtemplateare"private": trueand correctly excluded. - Traced the four scenarios through
auto-bump-packages.sh: shared input alone changed, per-package source alone changed, both changed, hand-bumped-only.HAS_BUMPS,SHARED_CHANGED, and per-package emission still behave correctly. - Cross-checked the
while read -r pkg_name; do ... done <<< "$PUBLISHABLE"construct: command substitution strips trailing newlines from the helper's output, the here-string re-appends one,readiterates cleanly with no empty trailing iteration. - Verified
printf '%s\n' "$PUBLISHABLE" | grep -qxF "$name"is safe against every package name in the tree (no metacharacters,-Ffor literal,-xfor full-line,-qfor quiet).
What I did NOT verify (unchanged from prior review)
- Did not re-read GreasyFork's Ruby source directly — the URL-shape claim from the original diagnosis still rests on the author's ported repro.
- Did not execute a real webhook delivery (only possible post-merge, as the PR body notes).
Recommendation
APPROVE. All four P2s from the prior review are addressed cleanly, one adjacent silent-degrade path the prior review had missed was caught and fixed in the same commit, no new issues introduced. Ready to merge, subject to the post-merge GreasyFork "sync from URL" step in the PR body.
Follow-ups:
- P0: none
- P1: none
- P2: none
- Post-merge (from PR body, not code changes; unchanged since prior review): set each script's GreasyFork "sync from URL" to
https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js, then confirm the first release webhook delivery returns a non-emptyupdated_scripts.
- Workflow run: https://github.com/nsheaps/.ai-agent-henry/actions/runs/32271904632
- Prior review (now minimised as outdated): #pullrequestreview-4973869501
- Related prior PRs: #40 (the regression), #48 (the derive-from-package-dir refactor kept here)
What changed
Three commits. The first fixes the URL, the second makes that fix actually ship, the third addresses review feedback.
0a23e5b— restore the release-asset URL form GreasyFork sync requiresscripts/build-userscript.mjs:31-39,58-62@downloadURL/@updateURLare derived ashttps://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.jsagain (was araw.githubusercontent.com/.../latest/packages/<pkg>/dist/script.user.jsblob URL). Keeps #48's derive-from-the-package-directory approach — only the URL shape changes..github/workflows/release.yaml:20-69.github/workflows/release.yaml(Commit built userscripts for GreasyFork sync)<pkg>.user.jsmirror alongside the nestedpackages/<pkg>/dist/script.user.jscopy..github/workflows/release.yaml(Upload userscripts as release assets)<pkg>.user.js..github/workflows/release.yaml(Tag release)latesttag stays and still moves — comment corrected to say why (scripts installed via the raw-blob URL during the #40 window keep updating through it).README.md:58-89f992ae1— treat a build-script change as changing every published packagescripts/auto-bump-packages.sh(SHARED_BUILD_INPUTS)scripts/build-userscript.mjsis declared a shared build input: when it differs from the change base, every opted-in package counts as changed. Per-package detection is otherwise untouched.9d7aef6— address review (all four P2 nits)scripts/publishable-packages.sh(new,100755)greasyforkPublishopt-in. The check was spelled out in three places (both release.yaml steps +auto-bump-packages.sh); a half-applied change to it would have shipped silently.scripts/auto-bump-packages.shSHARED_BUILD_INPUTSentry is now verified to resolve atHEAD(git cat-file -e) before use — a pathspec matching nothing exits 0 with no output, so a rename would have silently re-armed the ship-nothing failure. Inline opt-in check replaced with the helper..github/workflows/release.yaml-ffrom the root-copygit add(only the nesteddist/path is ignored; comments now say which is which). Removed the deaddist-shaoutput and the now-unusedid: dist-commit. Both loops driven by the helper, captured into a variable (not process substitution) so a helper failure aborts underset -e, plus an explicit empty-list guard. Dropped the duplicated missing-artifact guard in the upload step.No script source or behaviour changes — nothing under
packages/*/src/is touched.Why
The break
GreasyFork stopped picking up new versions. #40 changed every published script's
@downloadURL/@updateURL— and with them the sync URL registered on GreasyFork — fromto
#40's reasoning covered only half of GreasyFork's contract. It is correct that
Github.file_from_root_for_urlreduces the raw-blob URL to the right git path (packages/<pkg>/dist/script.user.js) by discardinglatestas the ref segment. But matching a script is a round trip, not a one-way parse. On areleasewebhook with actionpublished, GreasyFork:file_from_root_for_url);urls_for_ref— once with the release's owntag_name, once with the repository'sdefault_branch— and selects scripts withsync_identifier IN (<those urls>), byte equality (inject_script_info,app/controllers/concerns/webhooks.rb);git show <tag>:<path>.Step 2 is the trap.
.../releases/latest/download/is the one base formurls_for_refspecial-cases to emit with no ref segment appended, so it round-trips back to itself. Every other supported form gets the release tag or the default branch substituted into the ref slot.latestis neitherrelease/<version>normain, so the stored URL was never among the regenerated candidates,process_webhook_changesfiltered everything out, and every delivery returned{"updated_scripts":[],"updated_failed":[],"message":"No scripts found."}.Step 3 is why the flat root-level mirror has to come back: the path from step 1 is
<pkg>.user.js, so that — not the nestedpackages/<pkg>/dist/path — is what GreasyFork asks git for at the release tag.The asset upload is a separate requirement: GreasyFork reads content from git, but userscript managers that installed straight from GitHub poll
@updateURLover plain HTTP, which only resolves if the asset is attached to the latest release.Why the second commit exists
The version-preview bot on the first push said it out loud:
no package changes detected. The URL fix touches onlyscripts/build-userscript.mjs, and change detection scopes its diff topackages/<pkg>/, so merging commit 1 alone would computehas_bumps=false, skip every conditional step in the release job, and ship nothing. The fix would have sat onmainlooking merged while GreasyFork stayed broken until somebody happened to touch a package.build-userscript.mjsrenders the metablock into every published artifact, so a change to it rewrites every artifact's bytes. Declaring it a shared build input makes that visible to change detection.Why the third commit exists
Review feedback (four P2 nits). Two of them are the same failure class as the bug this PR fixes — a check that degrades to "nothing to do" instead of failing — so they are worth more than tidiness: an unverified pathspec, and a helper invoked through process substitution where a non-zero exit would be swallowed.
Blast radius
github-actions-grafana-jump,github-to-graphite-button,graphite-to-github-button).https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js, GreasyFork will keep returning "No scripts found." no matter what this pipeline emits.latesttag and the nesteddist/path in the tagged commit, so URLs handed out during the grafana-jump: inline toolbar button, update toast, generic link targets #40 window keep resolving.30643f0). refactor(build): derive userscript @downloadURL/@updateURL from the package directory #48's "derive the URL from the package directory instead of hand-typing it inmeta.json" is preserved.Validation performed
The regression, reproduced. Ported GreasyFork's own
file_from_root_for_url/urls_for_ref/inject_script_infologic fromlib/github.rbandapp/controllers/concerns/webhooks.rband replayed both URL forms through it. This is the assertion that fails without the fix:Every published package's rendered URL is byte-identical to its last known-working (pre-#40) value:
Rendered header, with the metablock still the literal first line of the file:
Dry-ran the restored
Commit built userscripts for GreasyFork syncloop (git ops stubbed out) — opt-in detection is right and each mirror name matches that package's asset name exactly:The would-have-shipped-nothing bug, before and after. Before commit 2 (this is the failure the version-preview bot reported on the first push):
After:
No over-triggering. Per-package detection is unchanged when no shared input moved — checked against real history in a worktree at
215ef50, a commit that touches one package only:and a no-op diff still reports nothing:
Commit 3 (review fixes)
Each of the reviewer's three factual claims was verified before acting on it, rather than taken on faith:
The new guard fires where the old code went quiet (this is the assertion that fails without the fix):
The extracted helper returns exactly what the three inline copies did, and detection for this PR is unchanged after the refactor:
Syntax / structure:
Not validated here: an actual end-to-end webhook delivery. That needs a real release plus the account-side sync URL fixed, so it can only be confirmed after merge — see "Related" for the check to run.
What a reviewer should scrutinise
urls_for_refspecial-casing only bases ending inlatest/download/. If GreasyFork's currentmasterdiffers from what I read, the conclusion changes. The alternative I rejected was pointing the sync URL atraw.githubusercontent.com/<org>/<repo>/main/packages/<pkg>/dist/script.user.js— that would match via thedefault_brancharm of step 2, and step 3 would still read the file at the release tag. I rejected it becausedist/is never onmain, so that URL 404s for anyone installing directly from GitHub, and it would force@downloadURLand the sync URL to diverge. Worth a second opinion.workflow_dispatchwith a hand-pickedfrom_shaold enough to sweep all three packages in. I chose the durable fix over the one-off, because the "build-tooling change ships nothing" trap will otherwise bite again. Say so if you'd rather it were manual.SHARED_BUILD_INPUTS. I kept it to exactlyscripts/build-userscript.mjs— the one file whose content lands inside every built artifact — rather than something broader like "any change outsidepackages/". Broader would be safer against this class of bug and noisier in bumps. This is a judgement call, not a derived answer.latesttag. Nothing in the pipeline resolves through it any more. I kept it so installs made during the grafana-jump: inline toolbar button, update toast, generic link targets #40 window keep updating, at the cost of a tag with no first-party consumer. Dropping it is defensible.scripts/publishable-packages.shreturning nothing during a release run now hard-fails the step rather than committing/uploading nothing. That treats "a release was cut with no publishable packages" as impossible-by-construction. If there is a legitimate path to that state, the guard is wrong and should be a warning.Related
30643f0), which dropped the flat mirror and the asset upload.24cedcd) moved URL derivation intoscripts/build-userscript.mjs; that part is kept.https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js.https://greasyfork.org/users/<id>/webhookis still enabled and subscribed to Releases (the push event alone cannot carry these files — they never land onmain).updated_scripts, not"No scripts found.".