Skip to content

Fix GreasyFork webhook sync: restore the release-asset URL form - #59

Draft
nsheaps wants to merge 3 commits into
mainfrom
claude/greasemonkey-webhook-updates-hiq53o
Draft

Fix GreasyFork webhook sync: restore the release-asset URL form#59
nsheaps wants to merge 3 commits into
mainfrom
claude/greasemonkey-webhook-updates-hiq53o

Conversation

@nsheaps

@nsheaps nsheaps commented Aug 19, 2026

Copy link
Copy Markdown
Owner

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 requires

File Change
scripts/build-userscript.mjs:31-39,58-62 @downloadURL/@updateURL are derived as https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js again (was a raw.githubusercontent.com/.../latest/packages/<pkg>/dist/script.user.js blob URL). Keeps #48's derive-from-the-package-directory approach — only the URL shape changes.
.github/workflows/release.yaml:20-69 Rewrote the header comment to spell out GreasyFork's full sync contract, including the round-trip step #40 missed.
.github/workflows/release.yaml (Commit built userscripts for GreasyFork sync) Re-added the flat repo-root <pkg>.user.js mirror alongside the nested packages/<pkg>/dist/script.user.js copy.
.github/workflows/release.yaml (Upload userscripts as release assets) Re-added the step that attaches each built script to the release as <pkg>.user.js.
.github/workflows/release.yaml (Tag release) The floating latest tag 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-89 Releases section rewritten: the real URL, the GreasyFork "sync from URL" requirement, and why no other URL form works.

f992ae1 — treat a build-script change as changing every published package

File Change
scripts/auto-bump-packages.sh (SHARED_BUILD_INPUTS) scripts/build-userscript.mjs is 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)

File Change
scripts/publishable-packages.sh (new, 100755) Single source of truth for the greasyforkPublish opt-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.sh Each SHARED_BUILD_INPUTS entry is now verified to resolve at HEAD (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 Dropped the misleading -f from the root-copy git add (only the nested dist/ path is ignored; comments now say which is which). Removed the dead dist-sha output and the now-unused id: dist-commit. Both loops driven by the helper, captured into a variable (not process substitution) so a helper failure aborts under set -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 — 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 correct that Github.file_from_root_for_url reduces the raw-blob URL to the right git path (packages/<pkg>/dist/script.user.js) by discarding latest as the ref segment. But matching a script is a round trip, not a one-way parse. On a release webhook with action published, GreasyFork:

  1. reduces each stored sync URL for the repo to a repo-root-relative git path (file_from_root_for_url);
  2. regenerates candidate URLs from that path via urls_for_ref — once with the release's own tag_name, once with the repository's default_branch — and selects scripts with sync_identifier IN (<those urls>), byte equality (inject_script_info, app/controllers/concerns/webhooks.rb);
  3. only then reads content, server-side, with git show <tag>:<path>.

Step 2 is the trap. .../releases/latest/download/ is the one base form urls_for_ref special-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. latest is neither release/<version> nor main, so the stored URL was never among the regenerated candidates, process_webhook_changes filtered 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 nested packages/<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 @updateURL over 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 only scripts/build-userscript.mjs, and change detection scopes its diff to packages/<pkg>/, so merging commit 1 alone would compute has_bumps=false, skip every conditional step in the release job, and ship nothing. The fix would have sat on main looking merged while GreasyFork stayed broken until somebody happened to touch a package.

build-userscript.mjs renders 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

  • What is affected: this repo only, plus the three published scripts' update path (github-actions-grafana-jump, github-to-graphite-button, graphite-to-github-button).
  • When it takes effect: on merge. Because of commit 2, merging this PR itself triggers a release run that patch-bumps all three packages (0.2.13→0.2.14, 0.3.8→0.3.9, 0.3.11→0.3.12), rebuilds them with the corrected URLs, and reattaches the assets. That bump-on-merge is intentional — it is how the fix reaches users.
  • ⚠️ Requires one manual, out-of-repo step. The sync URL is an account-side setting on GreasyFork, not something this repo controls. Until each script's "sync from URL" is set back to 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.
  • What breaks if this is wrong: GreasyFork sync stays broken (i.e. status quo) — this cannot make it worse. Direct-from-GitHub installs are the one thing that could regress; mitigated by keeping the latest tag and the nested dist/ 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.
  • Rollback: revert the three commits. No data migration, no state. A released version cannot be un-bumped, but a stale patch bump is harmless.
  • What it overrides: the URL-shape decision made in grafana-jump: inline toolbar button, update toast, generic link targets #40 (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 in meta.json" is preserved.

Validation performed

The regression, reproduced. Ported GreasyFork's own file_from_root_for_url / urls_for_ref / inject_script_info logic from lib/github.rb and app/controllers/concerns/webhooks.rb and replayed both URL forms through it. This is the assertion that fails without the fix:

$ python3 gf_sim.py
BROKEN (post-#40 raw-at-latest)
  sync_identifier -> git path : packages/github-actions-grafana-jump/dist/script.user.js
  matches regenerated urls    : False  => No scripts found. (never syncs)

FIXED  (release asset)
  sync_identifier -> git path : github-actions-grafana-jump.user.js
  matches regenerated urls    : True  => SYNCS

Every published package's rendered URL is byte-identical to its last known-working (pre-#40) value:

$ for p in github-actions-grafana-jump github-to-graphite-button graphite-to-github-button; do
    (cd packages/$p && node ../../scripts/build-userscript.mjs)
    new=$(grep -m1 '@downloadURL' packages/$p/dist/script.user.js | awk '{print $3}')
    old=$(git show 30643f0^:packages/$p/src/meta.json | python3 -c 'import json,sys; print(json.load(sys.stdin)["downloadURL"])')
    [ "$new" = "$old" ] && echo "MATCH  $p  $new" || { echo "DIFFER $p"; echo "  new=$new"; echo "  old=$old"; }
  done
MATCH  github-actions-grafana-jump  https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/github-actions-grafana-jump.user.js
MATCH  github-to-graphite-button  https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/github-to-graphite-button.user.js
MATCH  graphite-to-github-button  https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/graphite-to-github-button.user.js

Rendered header, with the metablock still the literal first line of the file:

$ head -20 packages/github-actions-grafana-jump/dist/script.user.js
// ==UserScript==
// @name        GitHub Actions => Grafana jump button
// @version     0.2.13
...
// @license     MIT
// @downloadURL https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/github-actions-grafana-jump.user.js
// @updateURL   https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/github-actions-grafana-jump.user.js
// ==/UserScript==
"use strict";

Dry-ran the restored Commit built userscripts for GreasyFork sync loop (git ops stubbed out) — opt-in detection is right and each mirror name matches that package's asset name exactly:

$ <dist-commit loop, git echoed>
git add -f packages/github-actions-grafana-jump/dist/script.user.js
git add github-actions-grafana-jump.user.js
git add -f packages/github-to-graphite-button/dist/script.user.js
git add github-to-graphite-button.user.js
git add -f packages/graphite-to-github-button/dist/script.user.js
git add graphite-to-github-button.user.js

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):

$ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview
false
| Package | Base | New | Action |
|---|---|---|---|
| _no package changes detected_ |  |  |  |

After:

$ ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview
true
| Package | Base | New | Action |
|---|---|---|---|
| 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 |

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:

$ (cd <worktree@215ef50> && ./scripts/auto-bump-packages.sh --change-base=215ef50^ --version-base=215ef50^ --preview)
true
| Package | Base | New | Action |
|---|---|---|---|
| github-actions-grafana-jump | 0.2.12 | 0.2.13 | will-bump |

and a no-op diff still reports nothing:

$ (cd <worktree@main> && ./scripts/auto-bump-packages.sh --change-base=main --version-base=main --preview)
false
| _no package changes detected_ |  |  |  |

Commit 3 (review fixes)

Each of the reviewer's three factual claims was verified before acting on it, rather than taken on faith:

$ git diff --name-only HEAD~1..HEAD -- scripts/does-not-exist.mjs
$ echo "exit=$?"
exit=0                       # empty output + exit 0 => silent degrade CONFIRMED

$ git check-ignore -v github-to-graphite-button.user.js
$ echo "exit=$?"
exit=1                       # NOT ignored => the -f was unnecessary CONFIRMED

$ git check-ignore -v packages/github-to-graphite-button/dist/script.user.js
.gitignore:147:dist/	packages/github-to-graphite-button/dist/script.user.js
                             # nested path IS ignored => keep -f there

$ grep -rn 'dist-sha\|dist-commit' .github/
.github/workflows/release.yaml:233:        id: dist-commit
.github/workflows/release.yaml:268:          echo "dist-sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"
                             # only the definitions, no readers => dead CONFIRMED

The new guard fires where the old code went quiet (this is the assertion that fails without the fix):

$ 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 - update scripts/auto-bump-packages.sh
$ echo "exit=$?"
exit=1                       # previously: exit 0, SHARED_CHANGED=false, ship nothing

The extracted helper returns exactly what the three inline copies did, and detection for this PR is unchanged after the refactor:

$ diff old-optin.txt new-optin.txt   # no differences

$ ./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 |

Syntax / structure:

$ bash -n scripts/auto-bump-packages.sh scripts/publishable-packages.sh
bash syntax OK

$ python3 -c "import yaml; d=yaml.safe_load(open('.github/workflows/release.yaml')); [print('  -', s.get('name') or s.get('uses')) for s in d['jobs']['release']['steps']]"
  - Checkout as automation bot
  - Resolve base ref
  - jdx/mise-action@9e7f7633ff6f6d6048a9418a68d48f288f50eb14
  - Install dependencies
  - Bump changed packages
  - Lint after bump
  - Commit bumps
  - Build bumped packages
  - Commit built userscripts for GreasyFork sync
  - Tag release
  - Push commit and tags
  - Create the GitHub Release
  - Upload userscripts as release assets

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

  1. The claim that no other URL form can work. It rests on urls_for_ref special-casing only bases ending in latest/download/. If GreasyFork's current master differs from what I read, the conclusion changes. The alternative I rejected was pointing the sync URL at raw.githubusercontent.com/<org>/<repo>/main/packages/<pkg>/dist/script.user.js — that would match via the default_branch arm of step 2, and step 3 would still read the file at the release tag. I rejected it because dist/ is never on main, so that URL 404s for anyone installing directly from GitHub, and it would force @downloadURL and the sync URL to diverge. Worth a second opinion.
  2. Commit 2 bumps all three packages on merge. That is deliberate — it is the only way the corrected URLs get built and published — but it means a version bump for two scripts whose source did not change. The alternative is merging commit 1 alone and then firing workflow_dispatch with a hand-picked from_sha old 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.
  3. Scope of SHARED_BUILD_INPUTS. I kept it to exactly scripts/build-userscript.mjs — the one file whose content lands inside every built artifact — rather than something broader like "any change outside packages/". Broader would be safer against this class of bug and noisier in bumps. This is a judgement call, not a derived answer.
  4. Keeping the floating latest tag. 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.
  5. Restoring the flat repo-root mirror. It is genuinely ugly, and grafana-jump: inline toolbar button, update toast, generic link targets #40 removed it for good aesthetic reasons. It is load-bearing for step 3 and there is no way around it while the sync URL must be ref-free.
  6. The empty-list guard added in commit 3. scripts/publishable-packages.sh returning 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

  • Regression introduced by grafana-jump: inline toolbar button, update toast, generic link targets #40 (30643f0), which dropped the flat mirror and the asset upload.
  • refactor(build): derive userscript @downloadURL/@updateURL from the package directory #48 (24cedcd) moved URL derivation into scripts/build-userscript.mjs; that part is kept.
  • Post-merge checklist (cannot be done from the repo):
    1. On GreasyFork, set each of the three scripts' "sync from URL" to https://github.com/nsheaps/greasemonkey-scripts/releases/latest/download/<pkg>.user.js.
    2. Confirm the repo webhook to https://greasyfork.org/users/<id>/webhook is still enabled and subscribed to Releases (the push event alone cannot carry these files — they never land on main).
    3. Merging cuts a release automatically; check that delivery's response is a non-empty updated_scripts, not "No scripts found.".

…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.
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown

Userscript Version Preview

Preview only — versions and CHANGELOGs are bumped automatically on merge to main, not in this PR. Manual bumps to a higher version are preserved.

Package Base New Action
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

… 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_ |  |  |  |
@nsheaps nsheaps added the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label Aug 19, 2026 — with Claude
@automation-nsheaps automation-nsheaps Bot removed the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label Aug 19, 2026
henry-nsheaps[bot]

This comment was marked as outdated.

…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'))"
@nsheaps nsheaps added the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label Aug 19, 2026 — with Claude
@automation-nsheaps automation-nsheaps Bot removed the request-review Force an AI code review on a draft PR (open non-draft PRs review automatically) label Aug 19, 2026

@henry-nsheaps henry-nsheaps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

### ✅ 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

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.yaml at 9d7aef6 in full.
  • Verified scripts/publishable-packages.sh is 100755 via the git tree API.
  • Confirmed .gitignore line 147 covers only dist/, 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); _template and template are "private": true and 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, read iterates 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, -F for literal, -x for full-line, -q for 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-empty updated_scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants