fix(os): polish elizaOS live branding and update materialization#7797
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| git fetch --no-tags --depth=1 origin "${{ github.base_ref }}" | ||
| changed_files="$(git diff --name-only "origin/${{ github.base_ref }}...HEAD")" | ||
| git fetch --no-tags origin "${{ github.base_ref }}" |
There was a problem hiding this comment.
--depth, GitHub Actions' initial shallow clone is expanded to the complete history of the base branch, which can be hundreds of megabytes on an active repo. A bounded deepen (e.g. --deepen=100) is usually enough to resolve the merge-base while keeping fetch fast; the fallback path already handles the rare case where no common ancestor is found.
| git fetch --no-tags origin "${{ github.base_ref }}" | |
| git fetch --no-tags --deepen=100 origin "${{ github.base_ref }}" |
| with os.fdopen(src_fd, "rb", closefd=True) as src_handle, os.fdopen( | ||
| tmp_fd, | ||
| "wb", | ||
| closefd=True, | ||
| ) as dst_handle: | ||
| src_fd = -1 | ||
| for chunk in iter(lambda: src_handle.read(1024 * 1024), b""): | ||
| digest.update(chunk) | ||
| dst_handle.write(chunk) | ||
| actual = digest.hexdigest() | ||
| if actual.lower() != expected_sha256.lower(): | ||
| fail(f"verified runtime file changed while copying: {src}") | ||
| os.chmod(tmp_name, 0o755 if src_stat.st_mode & 0o111 else 0o644) | ||
| os.replace(tmp_name, dst) | ||
| tmp_name = None | ||
| finally: | ||
| if src_fd >= 0: | ||
| os.close(src_fd) | ||
| if tmp_name and os.path.exists(tmp_name): | ||
| os.unlink(tmp_name) |
There was a problem hiding this comment.
src_fd when dst_handle setup fails
with os.fdopen(src_fd, …) as src_handle, os.fdopen(tmp_fd, …) as dst_handle: is desugared as nested context managers. If os.fdopen(tmp_fd, "wb", …) raises after src_handle.__enter__() has succeeded, Python calls src_handle.__exit__(), which closes the underlying src_fd. The src_fd = -1 assignment in the body never executes, so the finally block then calls os.close(src_fd) on an already-closed descriptor — raising EBADF and masking the original exception.
| if tmp_name and os.path.exists(tmp_name): | ||
| os.unlink(tmp_name) |
There was a problem hiding this comment.
os.path.exists follows symlinks, so a dangling symlink planted at tmp_name (between os.replace failing and the finally running) would cause exists() to return False and silently skip cleanup, leaking the temp file. os.path.lexists checks the path itself without following symlinks, or a try/except around os.unlink is even more idiomatic.
| if tmp_name and os.path.exists(tmp_name): | |
| os.unlink(tmp_name) | |
| if tmp_name: | |
| try: | |
| os.unlink(tmp_name) | |
| except FileNotFoundError: | |
| pass |
2 participants
Summary
Validation
ELIZAOS_STATIC_SOURCE_ONLY=1 ./scripts/static-smoke.sh./scripts/security-smoke.shpasses with the existing production warnings for missing production update keyring and SBOM/provenance artifactsNotes
This keeps inherited Tails internals and package paths intact; it only changes safe visible polish and the elizaOS-owned update materialization path.
Greptile Summary
This PR rebrands first-boot and launcher UI strings from inherited Tails wording to elizaOS wording, and hardens the update-manager's file materialization path with
O_NOFOLLOWsource opens, temp-file + atomic-rename writes, and in-flight SHA-256 re-verification against the signed manifest.2000-aestheticshook, the desktop-directory label, and the launcher user-check message are all updated; a newrg-based regression guard instatic-smoke.shensures they stay clean.update-manager):copy_verified_filenow opens the source withO_NOFOLLOW, streams through atempfile.mkstemptemp file, re-hashes the content during the copy, and usesos.replacefor an atomic rename.Justfile): root discovery switches from fragile relativecdtogit rev-parse --show-toplevel, a newELIZAOS_MILADY_APP_ARTIFACTenv override is added, and anensure_plugin_runtime_disthelper builds plugin dists on demand.Confidence Score: 4/5
Safe to merge with two minor fixes; the materialization hardening is correct and the branding changes are straightforward.
The core security hardening in copy_verified_file is well-designed, but the compound with statement leaves a narrow double-close window on src_fd that masks exceptions, and the temp-file cleanup uses os.path.exists (symlink-following) rather than os.path.lexists/try-except. The CI fetch change drops the depth limit entirely, which can pull full branch history on every PR run.
The update-manager copy_verified_file function deserves the closest read, specifically the fd lifecycle between the compound with setup and the finally cleanup block.
Important Files Changed
Reviews (1): Last reviewed commit: "fix homepage smoke scope on shallow PR h..." | Re-trigger Greptile