Build-artifacts #13
Build-artifacts #13
Conversation
Co-authored-by: hey <[email protected]>
- Added commands to list running models and stop specific models or the server. - Improved model resolution logic to support running models by name. - Introduced a new module for managing the state of running models. - Updated README with detailed instructions on model management and server operations. - Added dependencies for `chrono` and other packages in `Cargo.toml` and `Cargo.lock`.
- Updated the CLI name from "tiles" to "Tiles" across all relevant files. - Enhanced the README with a quick start guide, installation instructions, and recommended setup for optimal usage. - Added support for a macOS agent to manage background processes and improve user experience. - Adjusted bundler and installer scripts to reflect the new CLI name and structure.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR adds packaging and installer tooling (ISO creation, macOS app bundling, Tiles Agent) and rewrites the interactive installer. CI workflow is modified to install a specific Rust toolchain action and to build & upload a tiles-installer ISO artifact. The CLI and runner are refactored: commands now accept model names, add list/stop/start operations, and resolve models via a registry. A persistent model state module tracks running models (models.json) with process liveness cleanup. Mem-agent/server integration and model background start/stop APIs are introduced, and server MODEL_ID is changed. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (src/main.rs)
participant Cmds as Commands (src/commands/mod.rs)
participant Registry as Model Registry
participant Runner as Runner (src/runner/mlx.rs)
participant Server
participant State as ModelState (src/runner/model_state.rs)
participant Proc as Background Process
User->>CLI: tiles run my-model
CLI->>Cmds: run("my-model")
Cmds->>Registry: resolve_model_to_modelfile("my-model")
Registry-->>Cmds: modelfile_path
Cmds->>Runner: start_model_background("my-model", modelfile)
Runner->>Server: is_server_running()?
alt server not running (non-debug)
Runner->>Server: start_server()
Server-->>Runner: ready
end
Runner->>Proc: spawn model process
Proc-->>Runner: pid
Runner->>State: load, add_model(name, id, pid), save
Runner-->>Cmds: success
Cmds-->>CLI: result
CLI-->>User: model started / error
sequenceDiagram
participant CI as GitHub Actions
participant Bundler as scripts/bundler.sh
participant MakeISO as scripts/make_iso.sh
participant Tools as xorriso/genisoimage/mkisofs
participant Artifacts as GitHub Artifacts
CI->>Bundler: build bundle (Tiles binary + assets)
Bundler-->>MakeISO: create dist/ bundle
MakeISO->>MakeISO: prepare .tiles-installer, README, macOS app (if present)
MakeISO->>Tools: package temp dir -> tiles-installer.iso
Tools-->>MakeISO: tiles-installer.iso
MakeISO->>Artifacts: upload tiles-installer.iso (artifact)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
@madclaws Ill be working on the build time issue: https://github.com/tilesprivacy/tilekit/actions/runs/19139490975/job/54700273885#step:6:1 feel free to suggest edits/suggestions here on this branch while im trying to get up to speed with the code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub async fn stop_model(model_name: &str) -> Result<(), String> { | ||
| use super::model_state::{get_state_file, ModelState}; | ||
|
|
||
| let state_file = get_state_file().map_err(|e| format!("Failed to get state file: {}", e))?; | ||
| let mut state = ModelState::load(&state_file) | ||
| .map_err(|e| format!("Failed to load model state: {}", e))?; | ||
|
|
||
| state.cleanup_stale(); | ||
|
|
||
| let _model = state.remove_model(model_name) | ||
| .ok_or_else(|| format!("Model '{}' is not running", model_name))?; | ||
|
|
||
| println!("✓ Stopped model '{}'", model_name); | ||
|
|
||
| // Save state | ||
| state.save(&state_file) | ||
| .map_err(|e| format!("Failed to save model state: {}", e))?; | ||
|
|
||
| // If no models are running, stop the server and agent | ||
| if state.is_empty() { | ||
| println!(" No models running, stopping server..."); | ||
| let _ = stop_server_daemon(); | ||
|
|
||
| // Stop the agent (macOS only, non-debug builds) | ||
| if cfg!(target_os = "macos") && !cfg!(debug_assertions) { | ||
| let _ = stop_agent(); | ||
| } |
There was a problem hiding this comment.
The new tiles stop command only edits the local ModelState JSON and prints a success message, but it never instructs the daemon to actually unload the model. In stop_model the code removes the entry from the state file and optionally shuts down the server when no entries remain, yet there is no HTTP call to /start//stop or any other API that would release the model when other models are still listed. As a result, stopping a specific model leaves it running inside the server and consuming resources until the entire daemon is killed. Consider invoking a server endpoint (or equivalent) to unload the model before updating the state file.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runner/mlx.rs (1)
1-520: Formatting check is failing.CI reports
cargo fmt --allfails on this file. Please run it locally to normalize formatting before merging.
🧹 Nitpick comments (2)
Cargo.toml (1)
6-8: Consider following Rust binary naming conventions.The binary name
"Tiles"uses uppercase, which is unconventional in the Rust ecosystem. Standard practice is to use lowercase names (e.g.,"tiles"). While this works technically, it may cause confusion for users familiar with Rust conventions.If conventional naming is preferred, apply this diff:
[[bin]] -name = "Tiles" +name = "tiles" path = "src/main.rs"Note: If the uppercase naming is intentional for branding purposes, this can be ignored.
README.md (1)
55-66: Add language specifier to fenced code block.The directory structure code block is missing a language identifier, which can affect rendering in some Markdown parsers.
Apply this diff:
-``` +```text ~/.tiles/ ├── registry/ # Model definitions
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_StoreCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/rust.yml(2 hunks).gitignore(1 hunks)Cargo.toml(2 hunks)README.md(2 hunks)ascii-art.txt(1 hunks)justfile(1 hunks)memgpt.modelfile(0 hunks)registry/memgpt/Modelfile(1 hunks)scripts/build-agent-app.sh(1 hunks)scripts/bundler.sh(2 hunks)scripts/install.sh(1 hunks)scripts/make_iso.sh(1 hunks)scripts/tiles-agent.sh(1 hunks)server/config.py(1 hunks)src/commands/mod.rs(2 hunks)src/main.rs(2 hunks)src/runner/mlx.rs(5 hunks)src/runner/mod.rs(1 hunks)src/runner/model_state.rs(1 hunks)
💤 Files with no reviewable changes (1)
- memgpt.modelfile
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{rs,toml}
⚙️ CodeRabbit configuration file
Review the Rust code for conformity with best practices in Rust, Systems programming. Highlight any deviations.
Files:
src/runner/mod.rsCargo.tomlsrc/runner/model_state.rssrc/commands/mod.rssrc/runner/mlx.rssrc/main.rs
🧬 Code graph analysis (7)
scripts/make_iso.sh (1)
scripts/install.sh (2)
log(93-95)err(97-101)
scripts/tiles-agent.sh (2)
scripts/install.sh (1)
log(93-95)scripts/make_iso.sh (1)
log(15-15)
src/runner/model_state.rs (1)
src/runner/mlx.rs (1)
get_config_dir(301-304)
src/commands/mod.rs (3)
src/runner/mlx.rs (5)
run(12-23)start_model_background(371-449)get_registry_dir(311-316)list_running_models(483-520)stop_model(451-481)src/core/modelfile.rs (1)
parse_from_file(246-251)src/runner/model_state.rs (1)
list_models(64-66)
scripts/install.sh (1)
scripts/make_iso.sh (1)
err(16-16)
src/runner/mlx.rs (2)
src/runner/model_state.rs (2)
get_state_file(99-102)load(21-33)src/commands/mod.rs (1)
stop_model(76-80)
src/main.rs (2)
src/runner/mlx.rs (2)
run(12-23)stop_model(451-481)src/commands/mod.rs (5)
run(11-30)list_models(70-74)stop_server(66-68)stop_model(76-80)start_server(62-64)
🪛 GitHub Actions: Rust
src/runner/mlx.rs
[error] 1-1: Rustfmt formatting check failed. Run 'cargo fmt --all' (or 'cargo fmt --all -- --check' to see issues) to fix formatting in this file.
🪛 LanguageTool
ascii-art.txt
[duplication] ~1-~1: Ebla mistajpaĵo: vi ripetis vorton
Context: ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~2-~2: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~4-~4: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~6-~6: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~8-~8: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~43-~43: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~45-~45: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~47-~47: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~49-~49: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~51-~51: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░...
(WORD_REPEAT_RULE)
[duplication] ~53-~53: Ebla mistajpaĵo: vi ripetis vorton
Context: ...░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░
(WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.18.1)
README.md
55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
scripts/tiles-agent.sh
[warning] 36-36: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 47-47: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (9)
.gitignore (1)
3-3: LGTM! Appropriate ignore pattern for build artifacts.The
dist/directory is correctly ignored to prevent committing generated ISO artifacts and installer distributions introduced by the new packaging workflow.Cargo.toml (1)
18-18: Good addition of chrono dependency.The
chronodependency is appropriate for timestamp management in the new model state tracking functionality.registry/memgpt/Modelfile (1)
1-1: LGTM! Consistent model image reference.The base image
driaforall/mem-agent-mlx-4bitaligns with the updatedMODEL_IDinserver/config.py, ensuring consistency across the model registry and server configuration.server/config.py (1)
4-4: LGTM! Consistent model ID update.The updated
MODEL_IDmatches the base image specified inregistry/memgpt/Modelfile, maintaining consistency between server configuration and registry definitions.ascii-art.txt (1)
1-55: LGTM! Static asset for installer UI.This ASCII art file is a static visual asset for the installation/packaging experience. The static analysis warnings about word repetition are false positives—the repeated characters are intentional box-drawing elements.
justfile (1)
23-24: LGTM! Clean build target for ISO generation.The new
isotarget follows the existing pattern and provides a convenient interface for generating ISO installers via themake_iso.shscript.src/runner/mod.rs (1)
2-2: LGTM! Clean module declaration.The new
model_statemodule is properly exposed and aligns with the PR's introduction of persistent model state tracking functionality.README.md (1)
4-151: Excellent documentation additions.The README now provides comprehensive guidance on installation, usage, directory structure, and model/server management. The documentation clearly explains both development and production workflows.
src/runner/model_state.rs (1)
20-78: Consider exposing async-safe path handling.The state management looks solid overall—nice use of JSON serialization, timestamping, and stale cleanup.
| [package] | ||
| name = "tiles" | ||
| version = "0.1.0" | ||
| edition = "2024" |
There was a problem hiding this comment.
Invalid Rust edition specified.
edition = "2024" is not a valid Rust edition. Valid editions are 2015, 2018, and 2021. This will cause compilation errors.
Apply this diff to use the latest stable edition:
-edition = "2024"
+edition = "2021"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| edition = "2024" | |
| edition = "2021" |
🤖 Prompt for AI Agents
In Cargo.toml around line 4, the edition is set to "2024", which is invalid;
update the edition field to a supported Rust edition (use "2021" for the latest
stable) by replacing edition = "2024" with edition = "2021" so the project
compiles with a valid Rust edition.
| if [[ -f "${LOCAL_BUNDLE}" ]]; then | ||
| print_info "Found local bundle" | ||
| cp "${LOCAL_BUNDLE}" "${TMPDIR}/tiles.tar.gz" | ||
| print_success "Bundle located" | ||
| elif [[ -f "${ROOT_BUNDLE}" ]]; then | ||
| print_info "Found bundle in repository" | ||
| cp "${ROOT_BUNDLE}" "${TMPDIR}/tiles.tar.gz" | ||
| print_success "Bundle located" | ||
| elif [[ "${ENV}" == "prod" ]]; then | ||
| print_info "Downloading from GitHub releases..." | ||
| TAR_URL="https://github.com/${REPO}/releases/download/${VERSION}/Tiles-v${VERSION}-${ARCH}-${OS}.tar.gz" | ||
|
|
||
| if curl -fsSL -o "${TMPDIR}/Tiles.tar.gz" "$TAR_URL"; then | ||
| print_success "Bundle downloaded" | ||
| else | ||
| err "Failed to download bundle from ${TAR_URL}" | ||
| fi | ||
| else | ||
| # Installer suppose to ran from tilekit root folder after running the bundler | ||
| mv "dist/tiles-v${VERSION}-${ARCH}-${OS}.tar.gz" "${TMPDIR}/tiles.tar.gz" | ||
| err "Could not locate bundle Tiles-v${VERSION}-${ARCH}-${OS}.tar.gz" | ||
| fi | ||
|
|
||
| echo "⬇️ Installing tiles..." | ||
| # Lets point to tile repo | ||
| tar -xzf "${TMPDIR}/tiles.tar.gz" -C "${TMPDIR}" | ||
| print_step "Extracting bundle..." | ||
| tar -xzf "${TMPDIR}/Tiles.tar.gz" -C "${TMPDIR}" | ||
| print_success "Bundle extracted" |
There was a problem hiding this comment.
Bundle name casing breaks local installs.
When a bundle is found locally/repo, you copy it to ${TMPDIR}/tiles.tar.gz (lowercase), but later extract ${TMPDIR}/Tiles.tar.gz (uppercase). That path never exists, so installs using a pre-downloaded bundle fail. Align the filename casing in both places (and in the curl path) before release.
🤖 Prompt for AI Agents
In scripts/install.sh around lines 162 to 185, the script copies local/repo
bundles to "${TMPDIR}/tiles.tar.gz" (lowercase) but later downloads/extracts
"${TMPDIR}/Tiles.tar.gz" (uppercase), causing a missing-file error; fix by using
a single consistent temp filename for the bundle throughout (choose either
Tiles.tar.gz or tiles.tar.gz) and update the cp targets, the curl -o path and
the tar -xzf path to use that same variable/path so the file written is the same
file read later.
| TILES_DIR="$HOME/.tiles" | ||
| MODELS_FILE="${TILES_DIR}/models.json" | ||
| AGENT_PID_FILE="${TILES_DIR}/agent.pid" | ||
| CHECK_INTERVAL=5 | ||
|
|
||
| # Logging | ||
| log() { | ||
| echo "[$(date '+%Y-%m-%d %H:%M:%S')] $*" >> "${TILES_DIR}/agent.log" | ||
| } | ||
|
|
||
| log "Tiles Agent started (PID: $$)" | ||
|
|
||
| # Save our PID | ||
| echo $$ > "${AGENT_PID_FILE}" | ||
|
|
||
| # Cleanup on exit | ||
| cleanup() { | ||
| log "Tiles Agent shutting down" | ||
| rm -f "${AGENT_PID_FILE}" |
There was a problem hiding this comment.
Create ~/.tiles before logging
log writes to ${TILES_DIR}/agent.log and the PID file is created immediately, but neither step ensures ${TILES_DIR} exists. On a fresh install this directory is missing, so the very first log write fails with “No such file or directory” and the agent exits because the script runs with set -euo pipefail. Please make the data directory before any logging or PID writes.
TILES_DIR="$HOME/.tiles"
MODELS_FILE="${TILES_DIR}/models.json"
AGENT_PID_FILE="${TILES_DIR}/agent.pid"
CHECK_INTERVAL=5
+# Ensure the workspace exists before we try to log or persist the PID.
+mkdir -p "${TILES_DIR}"
+
# Logging
log() {
echo "[$(date '+%Y-%m-%d %H:%M:%S')] $*" >> "${TILES_DIR}/agent.log"
}🤖 Prompt for AI Agents
In scripts/tiles-agent.sh around lines 6 to 24, the script writes logs and the
PID file into ${TILES_DIR} before ensuring that directory exists; on a fresh
install the initial log/PID write fails. Fix by creating the directory
immediately after TILES_DIR is defined (mkdir -p "${TILES_DIR}"), optionally set
safe permissions (e.g. chmod 700 "${TILES_DIR}"), and only then perform logging
and echoing the PID; ensure the mkdir command runs under the current
error-handling policy so failures surface.
| Stop { | ||
| /// Model name to stop (if not provided, stops the server) | ||
| model: Option<String>, | ||
| /// Stop the server daemon | ||
| #[arg(long)] | ||
| server: bool, | ||
| }, | ||
|
|
There was a problem hiding this comment.
Stop command help text contradicts actual behavior.
The help says omitting model “stops the server”, but the logic errors out unless --server is passed. Either default to stopping the server when model is None, or update the help/usage strings so users aren’t misled. Right now tiles stop prints the usage error, which is a functional UX bug.
🤖 Prompt for AI Agents
In src/main.rs around lines 41–48, the Stop command's help says omitting model
"stops the server" but the runtime requires --server; update behavior to match
help by treating model: None as an implicit request to stop the server: change
the command handling logic so that when Stop.model is None it proceeds to stop
the daemon (i.e., set server=true or call the server-stop path) instead of
erroring; ensure any usage strings remain the same and add a small comment
noting the implicit behavior so future maintainers understand the mapping.
… synchronization - Added rsync to the list of recommended tools in the README and installation script. - Updated the description to reflect that the tools ensure models are accessible, responsive, and synced 24/7.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main.rs (1)
37-48: Stop command help text still contradicts implementation.The doc comment on line 43 states "if not provided, stops the server," but the actual implementation (lines 92-95) prints an error and requires an explicit
--serverflag. This creates user confusion whentiles stopfails instead of stopping the server as documented.Either update the help text to match the current behavior, or change the implementation to treat
model: None(when--serveris not provided) as an implicit server stop:/// Stops a running model or the server /// /// Examples: /// tiles stop memgpt # Stops the memgpt model /// tiles stop --server # Stops the server (if no models are running) Stop { - /// Model name to stop (if not provided, stops the server) + /// Model name to stop (required unless --server is used) model: Option<String>, /// Stop the server daemon #[arg(long)] server: bool, },scripts/install.sh (1)
162-184: Fix bundle filename casing inconsistency—still broken from previous review.Local and repository bundles are copied to
${TMPDIR}/tiles.tar.gz(lowercase), but the extraction on line 184 attempts to read${TMPDIR}/Tiles.tar.gz(uppercase). This breaks installations using local/repo bundles.Align all bundle filenames to lowercase
tiles.tar.gzthroughout the script (copy destinations, download destination, and extraction source).Apply this diff:
-if curl -fsSL -o "${TMPDIR}/Tiles.tar.gz" "$TAR_URL"; then +if curl -fsSL -o "${TMPDIR}/tiles.tar.gz" "$TAR_URL"; then print_success "Bundle downloaded" else err "Failed to download bundle from ${TAR_URL}" fi -tar -xzf "${TMPDIR}/Tiles.tar.gz" -C "${TMPDIR}" +tar -xzf "${TMPDIR}/tiles.tar.gz" -C "${TMPDIR}"
🧹 Nitpick comments (5)
src/main.rs (1)
81-99: Consider passing string slice directly.On line 82,
model.as_str()is called to convertStringto&str. SinceStringimplementsDeref<Target = str>, you can pass&modeldirectly to avoid the explicit conversion:Commands::Run { model } => { - commands::run(model.as_str()).await; + commands::run(&model).await; }This is more idiomatic Rust and relies on automatic deref coercion.
README.md (2)
56-67: Add language specifier to fenced code block.The directory structure code block at line 56 is missing a language identifier. While bash is the common choice for
.tiles/directory trees,txtor no specifier is also acceptable depending on style preference.Apply this diff to improve linting compliance:
-``` +```txt
118-123: Fix capitalization for consistency with install destination.Lines 118 and 122 use lowercase
tilescommands, but the installer places the binary asTiles(uppercase). Update these to match the actual binary name for user clarity.Apply this diff:
-tiles start # Production +Tiles start # Production # Stop server manually (only if no models running) -cargo run stop --server # Dev -tiles stop --server # Production +cargo run stop --server # Dev +Tiles stop --server # Productionscripts/install.sh (2)
15-31: ASCII art fallback logic could be consolidated.The three nested
ifstatements searching forascii-art.txtacross multiple paths work correctly but could use a loop for better maintainability.Consider this alternative:
ASCII_ART_FILE="" for path in "${SCRIPT_DIR}/ascii-art.txt" "${SCRIPT_DIR}/../ascii-art.txt" "${SCRIPT_DIR}/../ascii-art.txt"; do if [[ -f "$path" ]]; then ASCII_ART_FILE="$path" break fi done if [[ -f "${ASCII_ART_FILE}" ]]; then cat "${ASCII_ART_FILE}" echo "" fi(Alternatively, keep the current approach if clarity is preferred over conciseness.)
93-95: Consider consolidating redundant UI functions.The
logfunction (lines 93–95) appears to duplicateprint_headerstyling. If they serve different purposes, consider renaming or documenting the distinction. If not, one function can be removed.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)scripts/install.sh(1 hunks)src/main.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{rs,toml}
⚙️ CodeRabbit configuration file
Review the Rust code for conformity with best practices in Rust, Systems programming. Highlight any deviations.
Files:
src/main.rs
🧬 Code graph analysis (2)
scripts/install.sh (1)
scripts/make_iso.sh (1)
err(16-16)
src/main.rs (3)
src/commands/mod.rs (5)
run(11-30)list_models(70-74)stop_server(66-68)stop_model(76-80)start_server(62-64)src/runner/mlx.rs (2)
run(12-23)stop_model(451-481)src/runner/model_state.rs (1)
list_models(64-66)
🪛 markdownlint-cli2 (0.18.1)
README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
src/main.rs (5)
6-18: Comprehensive CLI help text improves UX.The expanded help text with usage examples and setup recommendations is well-structured and provides clear guidance. The multi-line formatting is clean and the examples are actionable.
26-32: Well-documented command with clear intent.The transition from file path to model name simplifies the user experience, and the doc comments clearly explain the registry structure with a helpful example.
34-35: Simple and straightforward command.
50-51: Clean command definition.
56-57: Good deprecation approach for backward compatibility.Hiding the command while keeping it functional allows existing scripts to continue working while steering new users toward the recommended
start/stopcommands.scripts/install.sh (3)
195-214: Approve CLI installation and Python environment setup.The installation logic properly creates directories, installs the binary with executable permissions (755), and sets up the Python environment with
uv sync --frozen. Error handling is appropriate.
217-229: macOS agent installation logic is sound.Proper directory creation, safe removal of old versions, and correct permission handling for the agent script.
240-291: Excellent user experience in installation completion.Clear completion messaging, PATH setup detection and guidance, help display with fallbacks, and recommended tools section all contribute to a polished installation experience.
| print_step "Checking Python installation..." | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| PY_VERSION=$(python3 --version 2>&1 | awk '{print $2}') | ||
| print_success "Python ${PY_VERSION} found" | ||
| else | ||
| print_warning "Python 3.10+ not found" | ||
|
|
||
| if [[ "$OS" == "darwin" ]]; then | ||
| log "Installing via Homebrew..." | ||
| brew install python || err "Could not install Python automatically. Please install manually." | ||
| print_info "Installing Python via Homebrew..." | ||
| brew install python || err "Could not install Python. Please install manually from https://www.python.org" | ||
| elif [[ -f /etc/debian_version ]]; then | ||
| log "Installing via apt..." | ||
| print_info "Installing Python via apt..." | ||
| sudo apt-get update -y && sudo apt-get install -y python3 python3-venv | ||
| else | ||
| err "Please install Python manually: https://www.python.org/downloads/" | ||
| fi | ||
|
|
||
| print_success "Python installed" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Python version requirement mismatch between code and messaging.
Line 123 warns "Python 3.10+ not found," but the version check on line 119 uses python3 --version without verifying the version number (3.10+). If Python 3.9 is installed, the check succeeds and installation proceeds, but the warning suggests 3.10+ is required. Either enforce the version requirement or clarify the minimum version.
Suggested fix: Add explicit version check:
PY_VERSION=$(python3 --version 2>&1 | awk '{print $2}')
# Parse major.minor from PY_VERSION and compare
if [[ $(echo "$PY_VERSION" | cut -d. -f1,2) < "3.10" ]]; then
# trigger installation
fiAlternatively, remove the "3.10+" from the warning if earlier versions are acceptable.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for Python version references in install.sh
rg -n "python|3\." scripts/install.sh -A 2 -B 2
Length of output: 764
🏁 Script executed:
#!/bin/bash
# Check for Python version requirements in common config files
fd -e "txt" -e "md" -e "toml" -e "yaml" -e "yml" -e "json" -e "cfg" | head -20 | xargs grep -l -i "python" 2>/dev/null | head -5Length of output: 56
🏁 Script executed:
#!/bin/bash
# Look for version constraints or requirements documentation
cat README.md 2>/dev/null | grep -i -A 5 "python\|requirement\|version" | head -30Length of output: 1212
🏁 Script executed:
#!/bin/bash
# Check if there's a pyproject.toml with Python version specification
cat pyproject.toml 2>/dev/null | grep -i "python\|version" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check server/pyproject.toml for Python version specification
cat server/pyproject.toml 2>/dev/null | grep -i "python" -A 2 -B 2Length of output: 137
🏁 Script executed:
#!/bin/bash
# Get full install.sh context after line 136
tail -n +137 scripts/install.sh | head -30Length of output: 975
🏁 Script executed:
#!/bin/bash
# Search entire codebase for Python version validation or constraints
rg "python.*version|version.*python|3\.[0-9]" -i --type sh --type py | grep -v ".pyc" | head -20Length of output: 200
🏁 Script executed:
#!/bin/bash
# Check uv installation command and any version pinning
rg -n "uv|pip" scripts/install.sh -A 1 -B 1Length of output: 634
Add Python version validation to enforce >=3.10 requirement.
The code only checks for Python existence (line 119) but not version. Since server/pyproject.toml requires >=3.10, users installing Python 3.9 will see "Python 3.9.x found" and proceed successfully, only to encounter a cryptic error later at line 210 when uv sync fails on dependency resolution.
Add version comparison after line 120:
PY_VERSION=$(python3 --version 2>&1 | awk '{print $2}')
if ! python3 -c "import sys; sys.exit(0 if sys.version_info >= (3, 10) else 1)" 2>/dev/null; then
# Trigger installation block
fiAlternatively, remove "3.10+" from line 123 if earlier versions are acceptable (they are not—the requirement is hardcoded in pyproject.toml).
🤖 Prompt for AI Agents
In scripts/install.sh around lines 118 to 136, the script only checks for
python3 presence but not the version, so Python <3.10 can be accepted and cause
later failures; after detecting python3 (line ~119–120) run a version check
using python3 to ensure sys.version_info >= (3,10) and if that check fails enter
the same installation/error branch (install via brew/apt or show manual install
message) and update printed messages accordingly (e.g., don’t report success
unless the installed or existing python meets >=3.10); implement the check as a
conditional that triggers the existing install/error handling when the version
test fails.
Updated build, bundler, and ISO creation scripts to rename the macOS application bundle from 'Tiles Agent.app' to 'Tiles.app'. Adjusted related Info.plist fields, executable names, and log messages to reflect the new naming convention for consistency and clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/make_iso.sh (3)
21-23: Verify bundler.sh integration and error messaging.The bundle validation is correct, but if
bundler.shfails silently or produces unexpected output, the error on line 22 may not fully clarify what went wrong. Givenset -e, the script will exit ifbundler.shfails, which is safe—but consider whether line 19 should explicitly log the bundler invocation for visibility into the build process.
55-234: macOS AppleScript wrapper is complex—consider extracting for clarity.The macOS-specific logic (lines 56–234) handles multiple sub-tasks: creating
.commandscripts, building anInfo.plist, and wrapping the app in an AppleScript bundle. While the logic appears sound, the nesting and length make it harder to maintain and test independently. This is currently acceptable but could benefit from extraction into a separate helper function (e.g.,create_macos_app_wrapper()) in a future refactor.Additionally, lines 47–49 extract
Tiles.appfrom the bundle, and lines 117–234 then modify or overwrite it (at least itsInfo.plistand executable). Confirm that this is intentional and that the bundler output supports this pattern.
236-279: README generation has redundant template code.The two README templates (lines 238–257 and 261–278) are nearly identical, differing only in version placeholder format (literal
${VERSION}vs.<version>). Consider consolidating this using a helper function to reduce duplication and improve maintainability.Example refactor (optional):
create_readme() { local readme_path="$1" local ascii_art="${2:-}" local version="$3" { if [[ -n "${ascii_art}" ]]; then cat "${ascii_art}" echo "" fi cat <<EOF Welcome to Tiles Installer! INSTALLATION: Double-click "Tiles.app" to start the installation. ... EOF } > "${readme_path}" }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/build-agent-app.sh(1 hunks)scripts/bundler.sh(2 hunks)scripts/make_iso.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/build-agent-app.sh
- scripts/bundler.sh
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/make_iso.sh (2)
scripts/tiles-agent.sh (1)
log(12-14)scripts/install.sh (2)
log(93-95)err(97-101)
🔇 Additional comments (3)
scripts/make_iso.sh (3)
1-16: Good defensive bash practices.The use of
set -euo pipefail, proper trap-based cleanup, and consistent error handling establish a solid foundation. Logging style aligns well withscripts/install.sh.
38-53: Extraction and app placement logic is sound.Proper error handling for the tarball extraction, fallback when
Tiles.appis not found, and cleanup of temporary extract directory. The defensive logging on line 51 is helpful.
285-299: ISO creation with tool fallback is well-structured.The defensive approach of checking for
xorriso,genisoimage, ormkisofsin order is solid. The error message on line 297 could be slightly more actionable (e.g., suggest installation steps), but this is a minor polish.
Is |
chronoand other packages inCargo.tomlandCargo.lock.