feat(motd): add uwelcome + update umotd - #815
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds system tags, updates ChangesSystem Welcome Integration
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…pace Assisted-by: Claude Sonnet 4.6 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Root cause The merge queue has silently ejected every entry since 2026-07-06 (PRs #802, #815, #817, #824, #825 — nothing has merged in two weeks). common#660 added `if: github.event_name != 'merge_group'` to the **Export image for scanning** and **Scan image for CVEs** steps, but left these on `!= 'pull_request'` (which matches merge_group): - **Promote image to root storage** — reads `/tmp/scan-image.tar`, which the skipped export never produced: `open /tmp/scan-image.tar: no such file or directory` ([failing merge-group run](https://github.com/projectbluefin/common/actions/runs/29701193456)) - **Push image / Write digest / Upload digest / manifest job** — these would push per-arch tags and re-sign `:latest` from a throwaway queue ref if the promote step hadn't failed first ## Fix Merge queue lane is now build-only: every push-lane step carries `github.event_name != 'pull_request' && github.event_name != 'merge_group'`. The queue verifies the combined commit builds; pushing, digests, manifests, and signing stay exclusive to the push/dispatch lanes. Self-healing: merge_group runs execute the workflow from the merged ref, so this PR's own queue run uses the fixed YAML. ## Skill update (same PR) `docs/skills/ci-tooling.md` merge_group section extended with the build-only rule and the artifact-lane tracing checklist. ## Validation - actionlint + full pre-commit: pass - `just check`: pass Unblocks #824 (MIME defaults) and #825 (game-devices-udev checksum), plus the stalled Renovate queue. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented merge queue builds from attempting image promotion, publishing, manifest creation, or digest uploads when required artifacts are unavailable. * **Documentation** * Added guidance for correctly gating merge queue workflow steps and tracing artifact availability across build lanes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Jorge Castro <jorge@projectbluefin.io> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
castrojo
left a comment
There was a problem hiding this comment.
I don’t see a ship/install path for uwelcome in this repo. The new config switches the command to , but the image only appears to install/ship
�[38;5;104;1m Welcome to�[m�[38;5;104;1m Bluefin�[m
[Ci] �[38;5;104;1m ghcr.io/projectbluefin/dakota:latest �[m
_ Command │ Description
──────────────────────────────────────┼─────────────────────────────────────
�[38;5;104;1m ujust --choose �[m │ List all available commands
�[38;5;104;1m umotd toggle �[m │ Toggle this banner on/off
�[38;5;104;1m ujust bluefin-cli �[m │ Enable terminal bling
�[38;5;104;1m brew help �[m │ Manage command line packages
�[38;5;104;1mSupport indie game preservation and OSS developers!�[m — �]8;id=1207160287;https://patreon.com/hitsave��[1mJoin Hit Save!'s�[m�]8;;�
�]8;id=1207160287;https://patreon.com/hitsave��[1mPatreon�[m�]8;;� �[38;5;104;4m�]8;id=1207160287;https://patreon.com/hitsave�https://patreon.com/hitsave�]8;;��[m
• �]8;id=3541281617;https://issues.projectbluefin.io/��[1mReport an issue�[m�]8;;� �[38;5;104;4m�]8;id=3541281617;https://issues.projectbluefin.io/�https://issues.projectbluefin.io/�]8;;��[m
• �]8;id=3766875054;https://ask.projectbluefin.io/��[1mAsk Bluefin�[m�]8;;� �[38;5;104;4m�]8;id=3766875054;https://ask.projectbluefin.io/�https://ask.projectbluefin.io/�]8;;��[m
• �]8;id=772340340;https://docs.projectbluefin.io/��[1mDocumentation�[m�]8;;� �[38;5;104;4m�]8;id=772340340;https://docs.projectbluefin.io/�https://docs.projectbluefin.io/�]8;;��[m today. Please either wire up uwelcome in the image or keep the config aligned with the actually shipped binary so the new path is functional.
castrojo
left a comment
There was a problem hiding this comment.
I don’t see a ship/install path for uwelcome in this repo. The new config switches the command to uwelcome toggle, but the image only appears to install/ship umotd today. Please either wire up uwelcome in the image or keep the config aligned with the actually shipped binary so the new path is functional.
|
tbf I was expecting you to do your magic |
Vanguard Lab Strike Report
ResultsEvidence collected via
Merge-queue eligibility
Blockers / issues filed
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Containerfile (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an absolute path for consistency.
The destination path
out/shared/...is relative. While this works because the default working directory is/, it is inconsistent with other absolute paths used in this file (e.g.,/out/shared/...in line 37). Consider adding a leading slash to ensure correctness if the working directory ever changes.💡 Proposed refactor
- install -Dpm0644 "$f" "out/shared/usr/lib/udev/rules.d/71-${f##*/}"; \ + install -Dpm0644 "$f" "/out/shared/usr/lib/udev/rules.d/71-${f##*/}"; \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Containerfile` at line 35, Update the install destination in the Containerfile command to use the absolute /out/shared/usr/lib/udev/rules.d path, matching the other absolute output paths while leaving the source handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Containerfile`:
- Line 35: Update the install destination in the Containerfile command to use
the absolute /out/shared/usr/lib/udev/rules.d path, matching the other absolute
output paths while leaving the source handling unchanged.
|
It should be good now 👍 |
hanthor
left a comment
There was a problem hiding this comment.
Holding a neutral review — the build wiring looks coherent but I can't confirm the greeting path end-to-end from the diff alone, and there are a couple of concrete gaps worth resolving first:
-
uwelcomeis installed but nothing in the image invokes it. The config moves frometc/umotd/config.jsontoetc/uwelcome/config.jsonand the new binary lands at/usr/bin/uwelcome, but on this branch both greeting entrypoints —fish_greeting.fish(function fish_greeting; umotd; end) andprofile.d/umotd.sh— still callumotd, notuwelcome. If the newumotdis now "only prints translated messages" (per the PR description) anduwelcomeis the front-end that readsetc/uwelcome/config.json, then the greeting needs to switch touwelcome; otherwise the interactive welcome (commands/links) silently regresses. If insteadumotdstill delegates internally, please confirm it reads the newetc/uwelcome/path. -
default.justtoggle mismatch.umotd toggleindefault.justis unchanged, but the new config's toggle command isuwelcome toggle. These should agree. -
Supply chain:
uwelcomeis cloned from the personalgithub.com/themimolet/uwelcomerather than an org-owned repo (unlikeumotdfromprojectbluefin/motd). Intentional, or should it be migrated under the org before it ships in the image?
Minor: the Containerfile carries a lot of unrelated 2→4-space reindentation churn (the gdu checksum block, etc.). The values are unchanged — CodeRabbit's "refreshed SHA-256" note is a misread of the whitespace — but the noise makes the diff harder to audit; consider splitting it out. CI is green. Happy to approve once the greeting wiring (item 1) is confirmed.
|
For No. 1 I completely forgot. |
|
yeah my bad, i don't know why I didn't think about it |
hanthor
left a comment
There was a problem hiding this comment.
The uwelcome migration is a good direction, but there are two things to fix before this ships.
1. The fish migration will error on every shell start
system_files/shared/usr/share/fish/vendor_conf.d/fish_greeting.fish:
if test -e ~/.config/no-show-user-motd
mv ~/.config/no-show-user-motd ~/.config/uwelcome/disabled
endTwo problems, and they compound:
- There's no
2>/dev/nullhere, unlike the profile.d version. ~/.config/uwelcome/is not created anywhere in this PR. If it doesn't exist,mvfails.
So for any user who had opted out via no-show-user-motd, the mv fails, fish prints an mv error, and — because the source file is never removed — it fails again on every new fish shell, forever. The profile.d path has the same missing-directory bug; it just hides the error instead of showing it, which means the opt-out silently never migrates.
Both paths need the directory created first, e.g.:
if test -e ~/.config/no-show-user-motd
mkdir -p ~/.config/uwelcome
mv ~/.config/no-show-user-motd ~/.config/uwelcome/disabled 2>/dev/null
endand the equivalent in uwelcome.sh. Worth confirming against uwelcome upstream that ~/.config/uwelcome/disabled is actually the path it checks, since the whole migration hinges on it.
2. Two tests were commented out rather than fixed
In tests/test_motd_integration.bats:
# @test "uwelcome.sh: does not contain no-show-user-motd check" {
# @test "fish_greeting: does not check no-show-user-motd (opt-out delegated to uwelcome)" {These were live assertions in test_umotd_integration.bats and are now dead. I understand why — the migration shim does reference no-show-user-motd, so they'd fail as written. But leaving them commented out means the file's own header comment ("Neither profile.d nor fish_greeting should check for that file or gate the uwelcome invocation") no longer describes the code, and nothing enforces the shim's shape.
Please either delete them and update the header comment to describe the migration as it actually is, or rewrite them to assert what you now want — e.g. that the reference appears only inside the migration block and that uwelcome is invoked unconditionally afterward. A commented-out test is the one outcome that leaves no record of which it was.
Minor
tests/test_umotd_integration.batsalso dropped the threetoggle-user-motdrecipe tests along with the recipe itself. That's consistent (the recipe moved touwelcome togglein the config JSON), but users with muscle memory forujust toggle-user-motdnow get "recipe not found" with no pointer. Consider keeping a stub recipe that execsuwelcome toggle.- The
Containerfilereindentation is a large share of the diff and makes the substantive uwelcome-build-stage addition harder to spot. Not worth re-rolling for, just noting for next time.
Generated by Claude Code
hanthor
left a comment
There was a problem hiding this comment.
Following up my earlier review with actual patches — the two mkdir -p suggestions are click-to-apply, and the third replaces one of the commented-out tests with one that pins the bug.
I verified the failure against the branch rather than reading it off the diff: ~/.config/uwelcome/ is not created by anything in this PR, so on a machine where uwelcome has never written its config dir, the mv fails in both shells. The fish path shows the error every shell start; the bash path swallows it and silently leaves the user opted in despite their earlier opt-out.
Generated by Claude Code
….fish Co-authored-by: James Reilly <jreilly1821@gmail.com>
Co-authored-by: James Reilly <jreilly1821@gmail.com>
Co-authored-by: James Reilly <jreilly1821@gmail.com>
|
Thanks for the fixes and tests, I didn't think about that As for the supply chain issue you mentioned a while back now, I've invited Ahmed and Jorge on the repos and so they'll be able to transfer them whenever. |
I can help, can you go to settings and in the danger zone tell me if you can transfer ownership to the org? I don't have repository permissions to allow me to move them via GitHub. |
You'll need to give me more broad repository access then? I don't see the danger zone at all. |
|
Can you try inviting me as an admin? |
|
I moved them both, thanks again for all the work here @theMimolet |
|
Thanks a lot ! |


Good day 👋
PR pipeline
What does this change?
It replaces the old unified umotd config file by the two new ones for umotd and uwelcome.
Why?
To update them and make them work together.
(also so you can use umotd in other programs as it only prints translated messages)
Checklist
just checkpassespre-commit run --all-filespassesfix:,feat:,chore:, etc.)Summary by CodeRabbit
uwelcome toggle, including updated greeting structure and MOTD command grouping.uwelcomealongsideumotd, and refreshed bundled udev asset verification.