Blockerbot for Forge #299
No reviewers
Labels
No labels
ai-review-please
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
discussions
easyfix
enhancement
task
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
4 participants
Notifications
Due date
No due date set.
Blocks
#296 Switch from Pagure to Forge for blocker review discussions
quality/blockerbugs
Reference
quality/blockerbugs!299
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/296-blocker-review-voting-on-forge"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements blockerbugs app exension adding Forge support
ecc834176btoa1bd0f73be5feb437643to9cad52a4a2OK, my head officially exploded now. This is a massive patch. I decided to revert some unrelated changes, like formatting and other stuff, so that I have a chance to read the diff. I saved the previous branch locally and we can introduce those changes afterwards, when this massive PR is merged.
I also pushed
603872a5e6todevelopand rebased this PR, because those deps caused me lots of headaches.I'll continue adding comments to individual lines now.
I decided to keep the generated method docstrings in the PR, because they help with understanding. But I'm unable to check their correctness, definitely not all of them, so let's hope there's nothing too misleading in there.
So I hacked things up to run this through ai-code-review, and here's the result - it has a few suggestions that look superficially sensible, see what you think.
@ -77,0 +66,4 @@FORGEJO_URL = "https://forge.stg.fedoraproject.org/"FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"FORGEJO_REPO = "quality/blocker-review"FORGEJO_REPO_TOKEN = "YOUR SECRET FORGEJO API TOKEN"Unlike for Pagure, in Forge, we don't have repo-specific tokens, but just user tokens, that have access to all repos the user can access, without limitation. Is that correct? In that case maybe we could rename this to
FORGEJO_TOKENor evenFORGEJO_BOT_TOKENto make this clearer?Also, in a comment we could specify what permissions the token needs. Is it just
write:issueor something more?My note about adding a comment makes more sense to do in
setting.py.example, I guess.@ -77,0 +67,4 @@FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"FORGEJO_REPO = "quality/blocker-review"FORGEJO_REPO_TOKEN = "YOUR SECRET FORGEJO API TOKEN"FORGEJO_REPO_WEBHOOK_SECRET = "YOUR WEBHOOK SECRET"Maybe we should say in a comment that this webhook should be triggered on all issue events, here and/or in some documentation.
My note about adding a comment makes more sense to do in
setting.py.example.@ -157,0 +298,4 @@state = data.get("issue", {}).get("state", "")if not issue_number or not state:msg = "Unable to parse received message (issue number, state)"I don't see
databeing logged anywhere (or least bothissue_numberandstate), so it might be hard to reconstruct what happened here, if we don't print out the original message?@ -8,2 +10,4 @@from blockerbugs.models.bug import Bug# URL path component that identifies Forgejo issue linksFORGEJO_ISSUES_PATH = "/issues/"This is interesting. Do I understand correctly that you need to distinguish discussions that link to Pagure from discussions that link to Forge (so that when mass-closing them, you don't use Forge API calls to Pagure URLs), but you don't want to distinguish it based on hostname? In other words, is this here just because of the Pagure -> Forge transition, but that's the only purpose? If that is so, it might deserve a more explanatory comment.
What if we instead compared the stored discussion URL with
FORGEJO_URL + FORGEJO_REPOfrom settings, and made it a generally useful check? It would make sure it doesn't touch tickets from other repos than the specific one configured in project settings.@ -25,3 +53,2 @@def create_discussions_links(milestone, bugs, links_to_reuse={}):app.logger.info('Creating discussion tickets for %d bugs under %s' % (def create_forgejo_discussions_links(These function names received "forgejo" in their name when there were both pagure and forgejo implementations here. Now that we dropped Pagure, let's go back to their original names? So
create_discussions_links(). It will make it shorter and the diff simpler.@ -52,3 +106,2 @@def sync_discussions():if app.config["PAGURE_REPO_TOKEN"] in (bb_Config.PAGURE_REPO_TOKEN, ""):def sync_forgejo_discussions() -> None:This can be just
sync_discussions().@ -84,3 +176,1 @@not yet marked with ``Release.discussions_closed=True``.'''app.logger.info('Closing discussion tickets in inactive releases...')def close_forgejo_discussions(dry_run: bool = False) -> None:In this case, the "inactive releases" in name seemed pretty significant to me, because you see it right from the function name. I would keep original
close_discussions_inactive_releases()name.@ -77,0 +72,4 @@FORGEJO_BOT_ENABLED = TrueFORGEJO_BOT_LOOP_THRESHOLD = 2FORGEJO_ADMIN_ORG = "quality"FORGEJO_ADMIN_TEAM = "reviewers"In production we have the members team that probably should have this rights. So let's use
"members"here, and create the same group on stg, so that we again have it synchronized and don't need to change it.These two options should also be added to
settings.py.example, I think.Hmm, I'm struggling with this a bit. In production, we have two teams - Owners (who have admin access to the org) and members (who have admin access to all repos). In staging, we only have Owners. I created members, but I have no way to populate it - it requires a matching FAS group, it can't be populated manually. So in order to test this, I think we need:
a) ask Infra to create a staging setup that is the same as production setup, i.e. also map members to the right FAS group (on stg).
In production, I see these FAS groups:
b) In our staging config, use
FORGEJO_ADMIN_TEAM = "Owners"and hope that it will work fine once we deploy it to productionFurthermore (a separate issue), I saw that blockerbot was part of Owners on staging. We don't want this, that's a security issue. I dropped it from the respective FAS groups, logged in with the account, and the group was updated, it's no longer an owner. Instead, I added blockerbot as a collaborator with write access - that should be sufficient for it to be able to update tickets. We need to test this, though.
//Edit: Actually, maybe it doesn't need to be even a collaborator, maybe just the API token access is sufficient? We should try both.
//Edit2: No, that wouldn't make sense. The token is a user token, configured in user settings. The token allows to operate on objects that the user has access to. So access to a particular repo tickets must be granted. I'll document this.
I've created a ticket here:
infra/tickets#13232
@ -135,1 +136,3 @@"PAGURE_REPO", "PAGURE_BOT_USERNAME", "PAGURE_BOT_ENABLED", "PAGURE_URL", "PAGURE_API",additional_env_keys = ["FAS_ADMIN_GROUP", "FORGEJO_REPO_TOKEN", "FORGEJO_REPO_WEBHOOK_SECRET", "FORGEJO_REPO","FORGEJO_BOT_USERNAME", "FORGEJO_BOT_ENABLED", "FORGEJO_URL", "FORGEJO_API","FORGEJO_ADMIN_ORG", "FORGEJO_ADMIN_TEAM", "DISCUSSION_SYSTEM",I think
DISCUSSION_SYSTEMis a leftover from previous code and can be deleted. It's also present in some other files, not just here.@ -21,1 +16,3 @@PAGURE_BOT_USERNAME = 'blockerbot'FORGEJO_URL = "https://forge.stg.fedoraproject.org/"FORGEJO_API = "https://forge.stg.fedoraproject.org/api/v1/"FORGEJO_REPO = "quality/blocker-review-test"I renamed the repo to
quality/blocker-review, but forgot to change it here as well. Oh well, let's fix it.@ -0,0 +1,97 @@# BlockerBugs Application - High-Level WorkflowIs this a leftover that you created while trying to understand the code, and forgot to delete? Or do you think we should keep this in? I'm personally unsure. It contains a lot of really specific details which we go stale in time. And using AI people can generate a fresh similar document locally any time. It also contains some invalid info already (e.g. there's no
flask sync-bugscommand, that's not how it's executed), so I'd need to go through it a check it properly.So there is some usefulness, but I'd probably go and rewrite it manually, strip details, and make it valid for a longer time.
@ -36,2 +35,3 @@--env POSTGRESQL_DATABASE=blockerbugs \--publish 5432:5432 \docker.io/library/postgres:13quay.io/fedora/postgresql-13:latestWhat is the reason for switching this to the quay.io image? (I'm not saying it's wrong, I simply don't know anything about it).
Using images from Docker Hub is rate limited and slower than using our own infrastructure.
@ -43,3 +44,2 @@$ podman exec -it blockerbugsdb psql --host=localhost --username=postgres --command='select version();'Password for user postgres:$ podman exec -it blockerbugsdb psql --host=localhost --username=fedora --command='select version();'I can't make this work. If I create the container as documented above, then:
The original instructions (with the image pulled from docker.io) work fine.
@ -203,6 +204,136 @@ While the virtualenv is active, you can build all documentation with this commanYou can then inspect the resulting docs in ``build/docs/html``. Open ``index.html`` to see the home page. Not everything is built as HTML and linked from there, so check directory contents as well, or even the docs source directory at ``docs/source``.Forgejo IntegrationThis seems too chatty for a general help, did you intend to keep this in, or remove it?
I think most (or maybe all) of it doesn't really fit into
development.rstand instead should be partially moved toinstallation.rst(how to configure the app) andblockerbugscli.rst(the commands). Some other parts (the workflows and troubleshooting) could go to a new file, e.g.usage.md(with expectation that we will drop rst in the future and convert everything to md, I'm +1 to that). Thoughts?@ -15,3 +15,3 @@then select a desired milestone and for each proposed blocker/freeze exception you'll see a **Vote** link displayed, which will direct you to the matching discussion ticket.If a bug report is very complex and requires real-time communication instead of asynchronous ticket discussions, we'll tag the ticket with the [meeting tag](https://pagure.io/fedora-qa/blocker-review/issues?status=Open&tags=meeting) and the discussion will take place during a blocker bug meeting (see above) instead of here.If a bug report is very complex and requires real-time communication instead of asynchronous ticket discussions, we'll tag the ticket with the [meeting tag](https://forge.fedoraproject.org/quality/blocker-review/issues?status=Open&tags=meeting) and the discussion will take place during a blocker bug meeting (see above) instead of here.The new URL doesn't work - the repo doesn't exist, but if it did, Forge doesn't seem to be use to use tag names in HTTP params, only tag IDs. So we need to create the tag first, to be able provide correct URL. However, we don't really use the meeting tag at all, so... I'll just rewrite this paragraph and replace it with something that doesn't need that link.
Adjusted
@ -72,3 +72,3 @@### Administrative commandsMembers of the [fedora-qa](https://pagure.io/group/fedora-qa) Pagure group can issue administrative commands. Those commands are handled in the same way as voting commands, i.e. they need to be placed on a separate line and are case-insensitive. Those commands include:Members of the [fedora-qa](https://forge.fedoraproject.org/quality/) Forge organization can issue administrative commands. Those commands are handled in the same way as voting commands, i.e. they need to be placed on a separate line and are case-insensitive. Those commands include:It's now named Quality, not fedora-qa. I'll adjust it.
Adjusted
@ -0,0 +1,23 @@[pytest]I wonder why you needed to moved it out of
setup.cfg, doespytestno longer load it? It seems it still does, even though they have a warning there. I chosesetup.cfgback in the days, because it was the only option to have all configuration in a single file, instead of having lots of individual files for all the tools. If it no longer works, we can look into replacing it with something modern.Gemini claims that the current modern standard is
pyproject.toml, which is supported by all those tools exceptflake8🙁 It recommends to replaceflake8withruff.I see that you already created
pyproject.toml, so if you had issues with pytest configuration insetup.cfg, why not move it directly topyproject.toml?Having a pytest configuration in
setup.cfgrequires use of double % to properly escape pytest formatting strings for use in OpenShift environment. However using double % in format strings insetup.cfgbreaks pytest in local environment. Current workaround is to move pytest config out ofsetup.cfgto eitherpytest.iniorpyproject.toml. I usedpytest.inito have pytest config visibly separated from anything else but I have no problem to usepyproject.tomlas well. Will move the pytest config in there.@ -2,6 +2,7 @@# this automatically also pulls rope and flake8 (which pulls pyflakes, pycodestyle and mccabe)python-language-server[rope,flake8]mypytestcontainers ~= 4.14.0This will be dependent on the outcome of #96, it's the same problem as for testdays-web. Actually a bigger one, because testdays-web doesn't really need it (the database can be sqlite), but we're running a whole forgejo in a container. More on that topic separately.
I think you meant quality/testdays-web#96 , right?
I think I have a workable solution for that one - see quality/testdays-web#99 - we can just create Forge action service containers with Postgres and Forgejo and use those when running in CI.
@ -17,6 +17,7 @@ munchpsycopg[binary,pool]pypycurlpyforgejoShould we use
>=or~=here? I have two minds about it, I wonder what's your preferred approach.For development, I think it useful to run on the latest stuff, to notice any breakage as soon as possible.
For production, it's good to pin versions most probably with
~=, because that prevents things from breaking randomly and at any time. (Our production can be re-deployed at any time, without our action, just because somebody needs to re-run playbooks for some reason).The problem is that this same file is used for both development and production 😆 So until we have a better approach, I guess it's better to pin versions here as well, to make production stable.
WDYT?
FWIW, I don't ever like pinning versions (as a general policy) at all, because then you've just created a big problem for yourself: you need to come up with a policy and mechanism for updating the pins.
I prefer to just always use no version specification, or
>=if appropriate, and address issues as they arise. If you have a good test suite and a staging deployment, you have plenty of opportunities to catch and address issues with new versions of dependencies before they reach production, and then you're not in danger of getting stuck on ancient versions and having to do a huge catch-up commit.It's kinda the "Fedora vs RHEL" question, and we're Fedora, sooo...:D
Re-running the playbooks doesn't have to include rebuilding the container image. But another way to address this would perhaps be to have versioned tags, or a 'production' branch, with pinned dependencies, and have the production deployment done from that tag/branch;
developcould have unpinned dependencies?I'd also prefer to get rid of pins as much as possible, but I suppose at least production would deserve to have some.
We can have two requirements-[stg,prod].txt files and just feed the required name into PIP_REQUIREMENTS env var before starting deployment.
Another option apparently (according to Claude Code) is moving
setup.pyintopyproject.toml, defining dependency groups there and generating requirements.txt at runtime.But I'd suggest opening a separate issue for such an update.
Ok, let's deal with this some other time. Leaving
pyforgejounpinned is probably OK for the moment.WIP: Blockerbot for Forgeto Blockerbot for ForgeReady for review
Can we do the same thing as in testdays-web to make this work with both testcontainers and CI-provided containers? Or would you prefer to merge this first then do that in #300 ?
I sort of think it might be nice to do the opposite - merge #300 first so we have CI and AI review, then this PR would have to pass those checks. WDYT?
Agreed, this is good idea, let's do #300 first. Do you want me to update
conftest.py?Sorry, update it where? Here? #300 ? #300 should work correctly at present AFAIK. Then any adjustments to make the new test setup in this PR work with the CI setup from #300 should be done here.
Right, OK. My bad, got it mixed up with testdays-web updates.
So to sum it up the plan is to merge #300 first and then do CI service containers update here in this PR. No problem with that!
OK. @kparal do you agree with the plan?
34ed8bf956to2c1d5a18d2AI Code Review
📋 MR Summary
This MR implements support for the Forgejo platform in the blockerbugs app, replacing the previous Pagure integration. It introduces a new
forgejo_interfaceandforgejo_bot, updates webhook endpoints, and adjusts related configurations and sync logic.pagure_bot.py,pagure_interface.py) with Forgejo equivalents (forgejo_bot.py,forgejo_interface.py).PAGURE_*toFORGEJO_*and added team/org-based admin validation./api/v0/webhook/forgejofor handling issue comments from Forgejo.Detailed Code Review
The implementation is comprehensive and well-structured, successfully migrating the voting and webhook logic from Pagure to Forgejo. The transition leverages standard pyforgejo methods and introduces testcontainers for more robust testing.
The logic in the webhook and the API handlers appears solid, effectively parsing commands, checking HMAC signatures, and interacting with the database. The security aspect of verifying webhooks and validating users via the Forgejo API is well-handled.
No critical bugs or security vulnerabilities were identified in the core logic. Minor suggestions are mostly related to ensuring edge cases are fully handled.
✅ Summary
🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
pretty useless AI review! (note its positive assertions are usually not worth taking seriously; it's mainly useful when it notices a problem). Oh, well. the test failure looks significant.
Yes, the whole container scaffolding in CI workflow and
conftest.pyneeds to be implemented for tests to pass. I was waiting for #300 merge, will work on this now.2c1d5a18d281d0ed301eAI Code Review
📋 MR Summary
This merge request replaces Pagure integration with Forgejo for blocker bug review discussions and voting.
ForgejoInterfacewrapper utilizing thepyforgejolibrary.issue_commentevents via HMAC-SHA256.testcontainersfor Forgejo and PostgreSQL.Detailed Code Review
The migration to Forgejo appears well-thought-out and comprehensive. The implementation cleanly replaces the old Pagure code while maintaining the existing voting logic. The use of testcontainers for end-to-end testing of the Forgejo and Postgres integrations is a substantial improvement in the project's testing methodology. Overall, the logic appears sound and secure, properly validating Forgejo webhook signatures in a constant-time manner.
✅ Summary
request.get_data()instead ofrequest.datain the Flask webhook handler as a safer way to access the raw payload for HMAC verification, thoughrequest.dataworks for JSON payloads.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
Ready for review and stg testing
There's a lot of linter problems which I'd like to address after review is done as not to add even more noise to already huge PR.
yeah, we need to tackle linting separately.
@ -30,23 +30,24 @@ PostgreSQL should be used both for development and production. You can set it upRun a `Podman`_ container with the latest `PostgreSQL image`_ (replace ``your-password`` with an actual value)::podman run --detach --name blockerbugsdb \I currently have
blockerbugsdb(development) andblockerbugs-db(unit tests), which is somewhat confusing. I suggest we useblockerbugsdb(orblockerbugs-db, if you like it better) name for development, andblockerbugs-test-dbfor testing (which is the same pattern that testdays uses). Sounds OK?Fixed in
ec68cd72cf@ -36,2 +35,3 @@--env POSTGRESQL_DATABASE=blockerbugs \--publish 5432:5432 \docker.io/library/postgres:13quay.io/fedora/postgresql-13:latestCI and unit tests use
quay.io/fedora/postgresql-16, I assume this is supposed to say 16 as well?Btw, do we need to upgrade some OpenShift definition file somewhere as well?
the image deployed to openshift is built by
infra/ansible@2db94b4913/roles/openshift-apps/blockerbugs/templates/buildconfig.yml.j2, AIUI. that uses some sort of magic thing somewhere that knows how to build a python app container from a source repo. I don't think it uses the Dockerfile in the repo at all.oh, but more to the point, we use an external db:
value: "db01{{ env_suffix }}.{{datacenter}}.fedoraproject.org"so we're stuck with whatever postgres version db01 is (in staging and prod). Which, right now, is 16 in both:
stg is the same.
Fixed in
ec68cd72cfThanks for investigation, Adam.
@ -0,0 +43,4 @@VoteCommand = collections.namedtuple("VoteCommand", ["tracker", "vote"])VoteCommand.__doc__ = (VoteCommand.__doc__ or "") + ': A parsed vote command, e.g. tracker="betablocker", vote="+1"'The original code in
pagure_bot.pywas:I'm not sure exactly why, because we don't seem to use the documentation anywhere. Perhaps the idea was to have the documentation accessible also in runtime and not just through a comment. Why not.
The
namedtuplealways seem to create an initial doc:The new approach is perhaps more defensive but also much harder to read. So why not just do:
I'll adjust this.
@ -0,0 +37,4 @@)if not base_url.strip():raise ForgejoAPIException("FORGEJO_API cannot be empty")This whole input arg validation is really chatty, are you used to do it this way? I would personally (at most) do something like (for each arg):
Maybe I wouldn't even check for type, because it Python you sometimes want to mock objects and give it something that behaves as a string. In the worst case, it will crash during
PyforgejoApiinitialization a few lines later.I also chose a general ValueError instead of ForgejoAPIException, because this is not an API call failure. ValueError seems the usual choice when init args are wrong.
Even better to custom checking here (and in similar classes), we could add at least basic checking into the config loader code, so that all of it happens in a central place and all consumers can rely on valid values instead of doing their own checking. But that's not the point of this PR, of course, just saying.
One thing that's bitten me with using generic
ValueErrorbefore is that, later on, you might at least want to know whether the error is from blockerbugs or something else entirely - I useValueErrors in fedfind, but have sometimes regretted it in fedfind consumers, where I have to doexcept ValueErrorthen try to make sure it's theValueErrorfrom fedfind that I was kinda expecting and not some otherValueError.So - it might be worth having a custom
BBValueErrorthat inherits fromValueError, or whatever. Or not...Fixed in
95205bd802I'd leave parameter type checking in there for now. It might be useful troubleshooting. We can always remove it later if it causes any problems.
Yes, it would be probably worthwhile to refactor Config into a dataclass in some future update.
@adamwill wrote in #299 (comment):
Interesting. I always like read upon good practices 🙂 So far, it seems to me that in our specific case, the simplicity might be better, because we're not a library, and we only call the code ourselves. But Jaroslav already subclassed it, that's fine, it's just a bit more code to maintain.
@ -0,0 +2,4 @@Testcontainer fixtures (forgejo_container, postgres_container, etc.) have beenmoved to testing/conftest.py to make them available for all test modules includingintegration tests and end-to-end tests.It looks like we can drop this file?
Fixed in
6438eb98f9Once we deal with the nitpicks above, I believe this is ready. I have to admit that even though I tried to give it a thorough look, because this PR is very important, I couldn't go through all of it. Especially the test suite-related bits (conftest, test*) are just too long. Combined with lots of changes with the main code base, including formatting, generated docs, etc, and the release validation cycle, it's just all too much. I used to believe in copious documentation and proper docstrings etc, but in the AI age, I'm losing my belief 🙂 It's hard to keep the project in my head when everything is so... spacious. Brevity will be the virtue of the future 😆
Once this is final, I think we don't want to merge it to
developjust yet. Instead, we'll want to push this to staging and give it a proper try. That might depend on:If you know about any further requirements/tasks that need to happen, please say so.
We could probably consider opeaning a feature request for creating a Makefile which would simplify local development - similar to testdays-web setup.
This can be tested in staging right away. The required env variables can be added and edited in OpenShift console WebUI: Workloads -> Deployments -> blockerbugs -> "..." -> Edit Deployment.
I think that's expected to be done via ansible, like everything else. Keys / passwords / secrets need to be added to the sekrit private ansible thingy on batcave01 and then referenced as variables in the public playbooks, I believe. You can file an infra ticket to get this done - you have to specify what variable names you want to exist, and if the values need to be specific we have to set up some kind of secure channel to transfer them; if infra can just make them up, that's easier.
I mean the infra ticket is already in the works and linked from #296 but until it is done we can just tweak env vars manually. We do not have to wait for it to be reviewed and merged.
I've started focusing on infra/ansible#3195 . There also seems to be some progress in forge/forge#477 (although no replies yet), so hopefully this won't take long anymore.
forge/forge#477 is now done, we should be fully capable of testing this in stg env without any workarounds. Jaroslav, have you tried?
(I believe we need to first update the
stagingbranch tip and redeploy in openshift, though).I missed the info regarding forge/forge#477 being done so no testing yet. Will focus on that now.
Let me know if you need stg FAS groups adjustments, I haven't checked those yet.
@jgroman Based on #296 (comment) , you can please adjust the code to check Quality org membership instead of Quality Members team membership?
Requesting changes requires a reason (boo!), and I always realize only after providing a regular comment instead 🙂️
Sooo... how angry will you be if I propose the drop the caching feature that you just spent time to implement? 😬️ I know the old comment requested it... But thinking about it, we use AGREED or similar a few times per day at most. That's zero extra load for Forge. Caching means we get an extra code to maintain, and an extra configuration option. I think it's not quite worth it, I'd rather keep it simpler. Sorry! Wdyt?
OK, no problem with that!
This seems to be working properly now in our quick testing in staging. A 🫙️ of 🍪️ for @jgroman !
I'll merge this PR, and we can follow up with the remaining steps in #296.
16ec640ff4dc63589bdd62bd4242e15a6f2b5736