fix(mac): revive the .app build against current deps + static layout#1557
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR modifies ChangesBundle Packaging Update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| mkdir -p "$CONTENTS/Resources/taos/static" | ||
| cp -R "$REPO_ROOT/static"/. "$CONTENTS/Resources/taos/static/" | ||
| rm -rf "$CONTENTS/Resources/taos/static/desktop" | ||
| cp -R "$STAGING/frontend" "$CONTENTS/Resources/taos/static/desktop" |
There was a problem hiding this comment.
WARNING: cp -R "$STAGING/frontend" "$CONTENTS/Resources/taos/static/desktop" silently re-introduces the original /desktop 404 if the upstream build script ever nests the SPA under $STAGING/frontend/desktop/ instead of placing index.html at $STAGING/frontend/index.html. The fix here assumes a specific shape for $STAGING/frontend, but nothing in this script asserts it (no [ -f "$STAGING/frontend/index.html" ] guard, and the target is renamed to desktop/ regardless of what is inside). If the SPA layout changes upstream, this path regresses to a nested desktop/desktop/ and /desktop 404s again with no build-time signal.
Consider either asserting the SPA shape (e.g. guarding with [[ -f "$STAGING/frontend/index.html" ]]) or, more robustly, copying contents with cp -R "$STAGING/frontend"/. "$CONTENTS/Resources/taos/static/desktop/" so the SPA root is determined by index.html location, not by the directory's name.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Previously flagged, now resolved
Files Reviewed (1 file + 1 generated skipped)
Fix these issues in Kilo Cloud Previous Review Summary (commit f841935)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f841935)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 38.9K · Output: 6.6K · Cached: 586.8K |
| # SPA_DIR). Resources/frontend was never read by the server. | ||
| mkdir -p "$CONTENTS/Resources/taos/static" | ||
| cp -R "$REPO_ROOT/static"/. "$CONTENTS/Resources/taos/static/" | ||
| # Locate the SPA root by its index.html rather than trusting the directory |
There was a problem hiding this comment.
WARNING: No existence guard for $REPO_ROOT/static before cp -R.
The neighbouring blocks at lines 62 and 66 use [[ -d "$REPO_ROOT/data" ]] / [[ -d "$REPO_ROOT/app-catalog" ]] before staging, so a missing directory is silently skipped. This block unconditionally runs cp -R "$REPO_ROOT/static"/. "$CONTENTS/Resources/taos/static/". If the repo layout ever drops the static/ directory (or this script is invoked from a stale checkout), the glob "$REPO_ROOT/static"/. does not match and cp is called with the literal path plus / plus .; the rest of the script will then stage the SPA into a fresh static/desktop/ but the shared assets directory will be empty, producing a broken /desktop with missing icons/wallpapers/manifests and no build-time signal.
Mirror the app-catalog pattern and wrap in [[ -d "$REPO_ROOT/static" ]], or fail loudly with a descriptive error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| rm -rf "$CONTENTS/Resources/taos/static/desktop" | ||
| mkdir -p "$CONTENTS/Resources/taos/static/desktop" | ||
| cp -R "$SPA_SRC"/. "$CONTENTS/Resources/taos/static/desktop/" | ||
|
|
There was a problem hiding this comment.
WARNING: cp -R follows symlinks by default and does not preserve their targets.
cp -R "$SPA_SRC"/. "$CONTENTS/Resources/taos/static/desktop/" (and the matching cp -R "$REPO_ROOT/static"/. ... on line 76) will, for any symlink found inside $STAGING/frontend or $REPO_ROOT/static, follow the link and copy the target file contents into the bundle rather than copying the symlink itself. If the staging tree ever contains a symlink that points outside $STAGING/frontend (e.g. a shared node_modules shortcut to the host filesystem, a stray editor symlink, or an attacker-controlled checkout), arbitrary host files can be silently inlined into taOS.app and shipped to users. This also breaks reproducibility (the target file's contents are baked in rather than a stable symlink).
Use cp -RP (or cp -a) so symlinks are preserved as symlinks and followed targets are not pulled in.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| SPA_SRC="$STAGING/frontend/desktop" | ||
| else | ||
| echo "assemble_bundle: no SPA index.html under $STAGING/frontend" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
SUGGESTION: Error prefix assemble_bundle: is inconsistent with the rest of the script.
Lines 36 and 110 (and the argparse error on line 17 uses assemble_bundle.sh:) emit messages with the [assemble_bundle] / assemble_bundle.sh: prefix. This newly added diagnostic uses bare assemble_bundle:, which breaks log-grep conventions and makes it harder to filter build noise.
| exit 1 | |
| echo "[assemble_bundle] no SPA index.html under $STAGING/frontend" >&2 |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Two months of controller drift broke the C1 build pipeline: - Regenerate tinyagentos/requirements.lock from the current pyproject (argon2-cffi, sqlcipher3, matrix-nio, taosmd 0.4.0, pywebpush, etc.). The old lock predated the auth rewrite, so python -m tinyagentos died at import with ModuleNotFoundError: argon2. - assemble_bundle.sh: stage the frontend where the server actually reads it. The server serves PROJECT_DIR/static and the SPA from static/desktop (SPA_DIR), but the script copied it to Resources/frontend, which nothing reads — /desktop 404'd. Now copies repo static/ plus the SPA build into Resources/taos/static. Verified: unsigned build assembles taOS.app + DMG; launching the bundled binary boots the embedded server, /api/health and /desktop return 200, SPA JS + shared static assets load, SIGTERM reaps the Python child.
Fold review: the bundle assembler assumed the SPA lives directly at $STAGING/frontend and renamed it to desktop/ regardless of contents, so an upstream layout change nesting the SPA under frontend/desktop/ would silently reintroduce the /desktop 404. Now the SPA root is found by its index.html (direct or one level down) and the build fails loudly if neither exists.
jaylfc
force-pushed
the
fix/mac-app-rebuild
branch
from
f841935 to
ede3c1e
Compare
| echo "assemble_bundle: no SPA index.html under $STAGING/frontend" >&2 | ||
| exit 1 | ||
| fi | ||
| rm -rf "$CONTENTS/Resources/taos/static/desktop" |
There was a problem hiding this comment.
WARNING: rm -rf "$CONTENTS/Resources/taos/static/desktop" silently destroys any pre-existing static/desktop/ content that was just copied in from $REPO_ROOT/static on the line above. If the repo's static/desktop/ ever ships shared (non-SPA) assets — PWA icons, fonts, manifests used by other routes, or files from a prior layout — they are wiped on every build without notice. Guard the deletion (e.g. only remove the dir when SPA_SRC differs, or move the cp -R of repo static/ to happen after the SPA copy, or explicitly test -d and only remove when the path was newly created).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| elif [[ -f "$STAGING/frontend/desktop/index.html" ]]; then | ||
| SPA_SRC="$STAGING/frontend/desktop" | ||
| else | ||
| echo "assemble_bundle: no SPA index.html under $STAGING/frontend" >&2 |
There was a problem hiding this comment.
WARNING: The error string "no SPA index.html under $STAGING/frontend" is misleading: this block only checks two hard-coded paths ($STAGING/frontend/index.html and $STAGING/frontend/desktop/index.html) and never actually searches the directory tree. If upstream ever nests the SPA elsewhere (e.g. $STAGING/frontend/v2/desktop/index.html, or a dist/ step in front of index.html), the user sees the generic "no SPA index.html" message with no hint about which paths were probed. List the probed paths in the message, or fall back to a find before declaring failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
The Mac app (C1+A milestone, PR #269) had drifted ~2 months. A rebuild against current dev surfaced two mechanical breakages, both fixed:
tinyagentos/requirements.lockpredated the auth rewrite and other additions, so the bundled Python had no argon2 and the app died at import with ModuleNotFoundError. Lock regenerated via uv pip compile for py3.12/aarch64-apple-darwin (also drops stale absolute paths from the old header).mac/build/assemble_bundle.shstaged the frontend to Resources/frontend, but the server serves from static/ (SPA at static/desktop), so /desktop 404'd. It now stages repo static/ plus the SPA build into the location the server actually reads.Smoke-tested on Apple Silicon against the rebuilt bundle (launcher run directly): /api/health 200, /desktop 200 with real SPA HTML + JS + manifest, clean SIGTERM shutdown reaping the embedded Python child. No signing/notarization (those stages self-skip without a Dev ID; notarization needs Jay's Apple Developer credentials). The pinned python-build-standalone (3.12.13) and apple-container-cli (0.12.0) still resolve with matching SHAs, no checksum bumps needed.
Built + smoke-tested by a subagent to an orchestrator spec; the orchestrator re-ran the smoke test and confirmed the assembled arm64 bundle.
Summary by CodeRabbit