[mstomar125]

Closes #2781 (Part B; Part A landed in #3601).

The macOS and Windows installers ship nomic-embed-text-v1.5.Q8_0.gguf in the app bundle under resources/models/, but nothing read it. The daemon only looks in models/ under its state directory, so every fresh install ran with semantic search off. This PR implements option 1 from the Part B findings: on launch the app copies its bundled model into that directory when nothing is there yet.

Summary

New module packages/desktop-app/app-lib/src/bundled_model.rs, called once from the startup task in lib.rs. The call sits after the daemon-status: starting emit and before ensure_daemon_running, because the daemon looks for the model only when it starts.

provision_bundled_model(app) never fails, and it runs the file work in spawn_blocking. It:

1. resolves the bundled file with Tauri's BaseDirectory::Resource (resources/models/<file>);
2. resolves the target <home>/.nodespace/models/<file> with the daemon's home rule: NODESPACE_HOME when set (read with std::env::var, as the daemon does), else the user's home;
3. keeps anything already at the target. The check is symlink_metadata, so even a broken link or a file that fails verification is left alone;
4. treats a bundle without the model (a dev build) as a no-op that creates nothing;
5. otherwise copies to <file>.tmp-<pid> beside the target and fsyncs it:
   • on macOS with std::fs::copy, which clones on APFS;
   • elsewhere by creating the file and streaming into it. On Windows fs::copy would carry the installed file's read-only attribute over, and Windows flushes only through a handle opened for writing;
6. hashes the temp file against the pinned SHA-256;
7. renames it into place, unless something appeared at the target meanwhile, and fsyncs the directory. The check and the rename are two steps, so something written in the microseconds between them would be replaced. The only writer expected there is another launch's copy of the same verified bytes.

Any failure removes the temp file and is logged as a warning. The daemon then starts with semantic search off, and the next launch retries. Temp files left by a copy whose process died are removed by the next copy.

Integrity. The copy must pass the same pinned digest the daemon checks (ADR-058). The app never replaces an existing model, so if it placed a bad file, that file would stay there for good. The daemon still verifies at load: on first load it re-hashes the copy and writes its <model>.verified.json next to it in ~/.nodespace/models/. Nothing writes inside the signed bundle.

Pinned values are repeated, with drift tests. nodespace-nlp-engine owns the digest. Making it a normal dependency of app-lib, even with default-features = false (no llama.cpp), puts it under cargo clippy -p nodespace-app-lib -- -D warnings, because that also lints workspace-member dependencies, and nlp-engine has 13 dead-code warnings without its features. So the digest and file name are repeated as private constants. Tests read the other sources and fail if any of these drift:

• nlp-engine's EMBEDDING_MODEL_SHA256;
• the daemon's lookup in assembly.rs;
• every gh release download … resources/models line in release.yml;
• the resources/models/**/* entry in tauri.conf.json.

sha2 (0.10.9, already in the lockfile) is the only new dependency.

When the daemon picks the model up. On an install's first launch, ensure_daemon_running extracts the daemon sidecar into ~/.nodespace/bin/ and starts or restarts the daemon, so the copy is in place before that daemon starts. That includes the .pkg, whose postinstall starts a daemon before the app first runs. If a copy happens while a healthy daemon with an unchanged binary keeps running, that daemon loads the model at its next start.

Tests

.tools/bin/cargo-nextest nextest run -p nodespace-app-lib. All the new tests are in bundled_model::tests and use tiny placeholder files with their own digest. No model is loaded and nothing touches the GPU.

Test	Setup	Expected
copies_the_bundled_model_when_none_is_present	Placeholder in a bundle; empty home	Copied; target bytes equal the bundle; the model dir holds only the model
an_existing_model_is_never_overwritten	A different file already at the target	AlreadyPresent; bytes unchanged
a_broken_link_at_the_target_is_kept (unix)	A dangling symlink at the target	AlreadyPresent; still the same link
a_model_that_appears_during_the_copy_is_kept	copy_verified called with a file already at the target (a copy that started before it appeared)	AlreadyPresent; bytes unchanged; no temp file
a_failed_copy_leaves_no_partial_file	Expected digest is not the placeholder's	Error naming the SHA-256 mismatch; no model; no temp file
the_streamed_copy_is_writable_even_from_a_read_only_bundle (added in review)	stream_to_temp, the copy used off macOS, from a read-only bundle	Same bytes; the copy is not read-only and opens for writing
a_read_only_bundle_is_copied (added in review round 2)	Read-only placeholder bundle, through provision (the clone path on macOS)	Copied; same bytes
a_dead_copys_temporary_file_is_removed	A stale <file>.tmp-1 in the model dir	Copied; only the model remains
a_missing_bundled_model_is_a_no_op	Bundle path absent	NoBundledModel; nothing created in the home
nodespace_home_wins_over_the_user_home	Pure home rule	NODESPACE_HOME first, then the user home
the_copy_follows_nodespace_home	NODESPACE_HOME and HOME set to two scratch dirs (restored before asserting)	The model lands under NODESPACE_HOME; the stand-in user home stays empty
the_target_is_the_daemons_model_path		<home>/.nodespace/models/nomic-embed-text-v1.5.Q8_0.gguf
the_pinned_digest_matches_nlp_engines	Reads nlp-engine/src/config.rs	Contains the same digest
the_file_name_matches_the_daemon_and_the_release	Reads assembly.rs, release.yml, tauri.conf.json	Same file name in the daemon lookup and every bundling download; resources/models/**/* is bundled
startup_copies_the_model_before_it_starts_the_daemon	Reads lib.rs	The copy is awaited once, before ensure_daemon_running

Mutation checks

Each mutation was applied on its own, the module's 13 tests were run, and the file was restored from git.

Mutation	Tests that fail
M1 entry_exists always false	an_existing_model_is_never_overwritten, a_broken_link_at_the_target_is_kept, a_model_that_appears_during_the_copy_is_kept
M2 commit drops its re-check	a_model_that_appears_during_the_copy_is_kept
M3 symlink_metadata → metadata (follow links)	a_broken_link_at_the_target_is_kept
M4 no temp cleanup on failure	a_failed_copy_leaves_no_partial_file, a_model_that_appears_during_the_copy_is_kept
M5 digest not checked	a_failed_copy_leaves_no_partial_file
M6 stale temp files not removed	a_dead_copys_temporary_file_is_removed
M7 no rename	copies_the_bundled_model_when_none_is_present, a_dead_copys_temporary_file_is_removed, the_copy_follows_nodespace_home
M8 no bundled-file check	a_missing_bundled_model_is_a_no_op
M9 user home wins over NODESPACE_HOME	nodespace_home_wins_over_the_user_home, the_copy_follows_nodespace_home
M10 wrong state dir	the_target_is_the_daemons_model_path
M11 digest drifts by one character	the_pinned_digest_matches_nlp_engines
M12 file name drifts (.f16.gguf)	the_file_name_matches_the_daemon_and_the_release, the_target_is_the_daemons_model_path
M13 copy moved after the daemon start in lib.rs	startup_copies_the_model_before_it_starts_the_daemon
M14 (review round) stream_to_temp goes back to fs::copy and reopens for writing	the_streamed_copy_is_writable_even_from_a_read_only_bundle

| M15 (review round 2) clone_to_temp reopens for writing | a_read_only_bundle_is_copied |

All 14 mutations were re-run on the review-round code (head 22ecaa90), with the same results. M15 was checked on 41da40c8. Every one of the 15 tests fails under at least one mutation.

Checks

• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo fmt -p nodespace-app-lib --check: clean.
• bun run scripts/check-pro-boundary.ts: clean. The push check (lint) passed.
• Full nextest run -p nodespace-app-lib, excluding the two files that load the real Gemma chat model onto the GPU (ai_chat_send_to_idle_test, model_download_terminal_state_test): 374 passed, 6 skipped (head 22ecaa90).
  • The run used a scratch HOME and NODESPACE_HOME, with CARGO_HOME and RUSTUP_HOME pinned to the real toolchain.
  • The integration tests used a fresh copy of the merge gate's nodespaced through NODESPACED_TEST_BIN. It was built from the Resolve the embedding model under NODESPACE_HOME #3601 stack, whose daemon, agent, core, nlp-engine, types and proto sources match this branch's base 8827dc82, so llama.cpp wasn't rebuilt. The only daemon-side change since then is this PR's comment in nlp-engine/src/config.rs.
  • The first run's 7 skill_setup failures were this fresh worktree missing packages/skill/dist/install.js. After bun run --cwd packages/skill build, all 27 skill_setup tests passed.
  • No test daemon was left running.
• Per the shared-machine rules, bun run test:changed and bun run merge were not run. Both load the real embedding model.

Not verified

• Windows. Nothing ran on Windows. The code is platform-neutral, with Tauri resource resolution and std::fs throughout. The Windows-only branches use cfg!(), not #[cfg], so they are type-checked on macOS too:
  • Off macOS the copy is streamed into a new file and flushed through its write handle. That path is unit-tested on macOS too, including from a read-only bundle;
  • sync_dir is skipped, because File::open can't open a directory there.
    Not checked on a real install:
  • Tauri's resource directory layout (resources\models\ next to the exe);
  • whether Defender or the indexer briefly blocks the rename of a freshly written 146 MB file. A failure is logged, the temp file is removed, and the next launch retries. Retrying the rename was left out until a Windows run shows it is needed.
• A real installed .app. Not exercised. That would mean installing into /Applications and launching it against the real ~/.nodespace, which is off-limits on this shared machine. The resource path matches skill_setup.rs's existing resources/skill resolution, which works in shipped builds.

Out of scope (follow-ups worth filing)

• release.yml downloads the model with gh release download and checks no digest (:100-108, :343-351). ADR-058's build-time check lives in scripts/download-models.ts, which the release doesn't use. A corrupt bundle is now caught at copy time and never placed, but the release should fail instead of shipping it.
• The app's own daemon management (ensure_daemon_running: bin/, logs/, database/, socket) and the launchd plist ignore NODESPACE_HOME. This module follows the daemon's rule, so an app run with NODESPACE_HOME set writes the model only under that home.

Documentation

• Docs PR: https://github.com/NodeSpaceAI/nodespace-docs/pull/103, to merge together with this one. It adds:
  • Daemon Lifecycle § Embedding Model (new): where the daemon looks, when the app copies, how the copy is verified, failure behaviour, and that CLI-only installs ship no model.
  • Installer & Distribution § Embedding Model (new): the installers carry the model in the bundle and copy it nowhere; the app does it on launch.
  • ADR-045 §2: amended. "Pinned by the installer" assumed the bundle reached the daemon; it now does through this copy.
• Implements ADR-058 (pinned digest, verified-state cache beside the model).
• Daemon protocol § state paths already lists models/ under nodespace_dir() (Part A's docs) and is unchanged.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Review — PR #3611

Review Type: Initial Review

Scope: core PR #3611 (bundled_model.rs, lib.rs call site, sha2 dependency) and docs PR NodeSpaceAI/nodespace-docs#103. Reviewed from the code; nothing was built or run, per the shared-machine rules.

Requirements Check

Requirement (decision comment / PR body)	Status	Notes
App copies its bundled model from resources/models/ into the NodeSpace home's models/ on launch	✅	Source comes from BaseDirectory::Resource, the same way skill_setup.rs resolves resources/skill. Target is NODESPACE_HOME or the user's home, then STATE_DIR/models/nomic-embed-text-v1.5.Q8_0.gguf. That matches the daemon's nodespace_home()/nodespace_dir() + resolve_model_path() (assembly.rs:750-758), including the std::env::var UTF-8 handling.
Only when no model is there; never overwrites	✅	symlink_metadata keeps any entry, including a broken link or a file that fails verification. One residual window remains between the re-check and the rename (S2). In practice it is harmless.
Atomic	✅	Copies to a temp file in the same directory, checks it against the pinned digest, fsyncs the file, renames it, then fsyncs the directory. On every outcome except Copied the temp file is removed, which covers copy, hash, sync, re-check and rename errors as well as AlreadyPresent.
Never blocks startup	✅	It never fails, runs in the spawned startup task, and does the file work in spawn_blocking. On the launch that copies, it does delay the daemon start by one clone + 146 MB hash on APFS, or a full copy + hash on Windows. That is the right trade-off, because the daemon reads the model only when it starts. If the copy keeps failing (a corrupt bundle, or S1 on Windows), the delay comes back on every launch.
Copy happens before the daemon starts, including first launch and .pkg	✅	I traced this. On a fresh install, extract_sidecar_if_changed returns true because ~/.nodespace/bin/nodespaced is missing, so the daemon restarts after the copy. With the .pkg, kill_running_daemon does not signal the postinstall daemon, because its argv0 /usr/local/bin/nodespaced ≠ ~/.nodespace/bin/nodespaced. Instead, bootstrap_launchd_agent's first bootstrap fails on the already-loaded app.nodespace.daemon label, and its bootout + retry restarts the job from the new plist. So the claim holds, through the bootout path rather than the kill.
CLI-only installs ship no model	✅	Stated accurately in the docs PR and ADR-045.
Docs first, merged together	✅	docs#103.
Closes #2781	✅	Part A (#3601) merged on 2026-10-05, so this is the last part.
No #NNNN in code comments or docs content; no Pro/sync wording in core	✅	Grepped both diffs: clean.
Repeating the digest and file name, with drift tests	✅	Reasonable. nlp-engine depends on no nodespace-* crate, so a shared constant would need a new dependency between crates. scripts/download-models.ts already repeats the digest and guards it only with a comment, so these tests are stronger than what exists. The release.yml check matches both download lines (:104, :347), each on one line.

Code Review Findings

No 🔴 or 🟡 findings. All four are optional.

🟢 S1: Windows: two plausible ways the copy fails on every launch
📁 packages/desktop-app/app-lib/src/bundled_model.rs:256-263, :237

• Problem. The PR already lists Windows as unverified. Two specific risks are worth closing cheaply:
  1. sync_file reopens the temp file for writing. That fails if CopyFileExW carried FILE_ATTRIBUTE_READONLY over from the installed resource.
  2. fs::rename right after writing a 146 MB file is a known place for a transient ERROR_ACCESS_DENIED or sharing violation from Defender or the indexer. rustup and cargo both retry renames on Windows for this reason.
• Impact. Either way, every launch repeats the full copy and hash, deletes the result, and the model never lands. Semantic search stays off, as it is today, but each launch pays the delay.
• Fix.
  • On Windows, clear the read-only attribute on the temp file before reopening it. Use #[cfg(windows)], not cfg!, because clippy::permissions_set_readonly_false fires when the branch compiles on Unix.
  • Retry the rename a few times with a short backoff on PermissionDenied.
  • Or, at minimum, keep both in the Windows verification follow-up.
• Principle. Fail-safe behaviour on a platform the change wasn't tested on.

🟢 S2: The no-overwrite guarantee has a small check-then-act window
📁 bundled_model.rs:233-243

• Problem. commit checks entry_exists(target) and then calls fs::rename, which replaces an existing file on all three platforms (rename(2), and MoveFileExW with REPLACE_EXISTING). The window is microseconds. Any realistic concurrent writer, such as a second app process, writes the same pinned bytes, so this is harmless today. The module and the docs do promise "never replaces", though.
• Fix (optional). fs::hard_link(&temp, target) fails atomically with AlreadyExists when anything is there (APFS, NTFS, ext4). Remove the temp file afterwards and fsync the directory. That drops the separate re-check and needs no new dependency.
• Principle. Make the invariant hold by construction rather than by a narrow race window.

🟢 S3: Stale-temp cleanup contradicts the pid comment (Nit:)
📁 bundled_model.rs:191-214

• Problem. The temp_path doc says the pid "keeps two app processes … off each other's file". But remove_stale_temp_files deletes every <file>.tmp-*, including another live process's temp file in mid-copy. That copy then fails with a NotFound on hash or rename, which logs a warning, while the other copy succeeds. The outcome is harmless, but the comment overstates the guarantee.
• Fix. Reword it, for example: "the pid keeps names distinct; a concurrent copy can be removed by the other's cleanup, fail with a warning, and be retried next launch". Alternatively, skip temp files whose pid is still alive.
• Related, minor. Cleanup runs only on the copy path. A temp file orphaned by a crash stays if a model later appears by other means.

🟢 S4: The rotation comments still say "both" (Nit:)
📁 packages/nlp-engine/src/config.rs:17, scripts/download-models.ts:21-23

• Problem. Both comments say a model rotation must update both digests. There are now three copies, and the file name also appears in the daemon lookup, release.yml and tauri.conf.json. The drift test will fail loudly, but whoever rotates the model would only find out from that failing test.
• Fix. Name desktop-app/app-lib/src/bundled_model.rs in both comments. That is a one-line change each.
• Principle. Make every copy findable from the comment, not only from a failing test.

Docs PR #103 findings

Checked against the code, all accurate:

• the macOS and Windows resource layout;
• "checked against the pinned SHA-256 before load" and the .verified.json cache semantics (config.rs:147-187);
• the NLP model not found — semantic search disabled message;
• download:models writing to ~/.nodespace/models/ (it uses homedir());
• "nothing writes inside the signed bundle";
• the first-launch restart behaviour.

The ADR-045 amendment follows the existing convention: a status-line note as in ADR-065, and a > **Amended <date>.** block as in ADR-083.

🟢 D1: Say that the app reads NODESPACE_HOME from its own environment
📁 architecture/daemon-cli-lifecycle.md:63

• Problem. "The app resolves that path with the daemon's own rule, so it follows NODESPACE_HOME too" is right about the rule, but the rule runs in the app's environment. On macOS the launchd-started daemon does not inherit the app's NODESPACE_HOME: write_plist passes only NODESPACED_SOCKET, NODESPACE_UI_BINARY and the profile's service_env. ensure_daemon_running also uses the real home. So an app launched with NODESPACE_HOME set copies under that home, while its daemon looks under ~/.nodespace.
• Context. Writing under NODESPACE_HOME is the right call, because it keeps isolated runs out of the user's live state. The PR body already lists the daemon-management gap as out of scope.
• Fix. One sentence noting the macOS launchd caveat would save a confusing debugging session.

Nit D2: ADR-045 amendment wording
📁 decisions/045-do-not-sync-embeddings-regenerate-locally.md:78

• Problem. "Every shipped install that has a model therefore runs the same pinned Q8_0 file." That guarantee comes from ADR-058's load-time check, not from the copy. The copy deliberately keeps a pre-existing file that doesn't match, and the daemon then refuses it.
• Fix. Something like: "…and since the daemon refuses any file that fails the pinned digest (ADR-058), every shipped install that loads a model runs the same pinned Q8_0 file."

Follow-ups

The PR body lists two out-of-scope follow-ups, and I found no open issue for either:

• release.yml does not check the model's digest.
• The app's daemon management ignores NODESPACE_HOME.

Worth filing them at merge time so they don't stay only in this PR's description.

What's good

• Placement. The call sits right after the starting emit and before ensure_daemon_running. A source-ordering test pins that order.
• Never fatal. Failures are logged, and the file work stays off the async workers.
• Correct no-overwrite check. symlink_metadata keeps broken links. The bundle check comes before any directory is created, so a dev build creates nothing.
• Cleanup in one place. A single !matches!(result, Ok(Copied)) guard removes the temp file on every non-success outcome. Verify-before-rename plus a file and directory fsync matters here, because a bad file that got placed would stay for good.
• Home rule. It mirrors the daemon exactly and never falls back to the real home when NODESPACE_HOME is set, which is the class of bug Part A fixed.
• Disk cost. fs::copy clones on APFS, so on macOS the "model on disk twice" cost from the issue's option analysis mostly goes away.
• Tests. They are small and use placeholder bytes, with no model and no GPU. Each one is backed by a documented mutation. The drift tests are a real improvement over the comment-only guard on the existing repeated digest.
• Dependencies. The only new one is sha2 = "0.10", the same spec every other crate uses.

Recommendation

APPROVE

Correct, well-scoped and well-tested, and a net positive: fresh installs get semantic search instead of carrying 146 MB of dead weight. All findings are optional. S1 is the one most worth doing before the first Windows release that includes this.

🤖 Generated with Claude Code

[mstomar125]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[mstomar125]

Address review: response to the pragmatic review (round 1)

Code fixes are in 22ecaa90, docs fixes in nodespace-docs#103 at 6c71f66.

✅ S1.1 🟢: Windows read-only attribute

bundled_model.rs: the copy no longer reopens the temp file for writing after fs::copy.

• Off macOS, stream_to_temp creates the temp file, streams the bytes into it, and flushes through that same write handle. No attributes are carried over from the installed file, so neither a read-only attribute nor FlushFileBuffers's need for a write handle can fail it.
• On macOS, clone_to_temp keeps fs::copy (an APFS clone) and flushes through a read-only descriptor, which is all fsync needs on Unix.
• No lint involved. Neither path touches set_readonly(false), so clippy::permissions_set_readonly_false doesn't come up. Both are selected with cfg!, so every platform type-checks both.
• New test: the_streamed_copy_is_writable_even_from_a_read_only_bundle. It runs stream_to_temp from a read-only bundle and checks that the copy has the same bytes, is not read-only, and opens for writing. It runs on macOS, so the Windows path is exercised here too.
• New mutation M14: stream_to_temp goes back to fs::copy and reopens for writing. The new test fails.

⏭️ S1.2 🟢: rename retry on Windows (skipped)

• No Windows run has shown the transient lock happens for this file.
• Each step before the rename (hash read, fsync) gives a scanner time to finish.
• A failure is already non-fatal: the temp file is removed, a warning is logged, and the next launch retries.
• Adding a retry loop now would mean untested code for an unobserved failure.
• Revisit if the Windows verification run logs cannot rename … to …. The PR body's "Not verified" section now names this.

⏭️ S2 🟢: hard_link for strict no-clobber (skipped, documented)

• Why not hard_link. It fails on filesystems without hard links: exFAT, and ReFS, which is what a Windows Dev Drive uses. There, the copy would then fail on every launch.
• The race is benign. The window is microseconds, and the only writer expected at that path is another launch's copy of the same verified bytes.
• Documented. commit's doc comment now states the window and why it is harmless. The PR body says the same.

✅ S3 🟢 Nit: pid comment

temp_path's doc now says the pid keeps the names of two concurrent copies apart, and that one copy's stale-file cleanup can still remove the other's file part-way. That copy then fails with a warning and the next launch retries.

• The related point is left as is: a temp file orphaned by a crash stays when a model later appears some other way. That needs a crash during the copy window and then a hand-placed model.

✅ S4 🟢 Nit: rotation comments

nlp-engine/src/config.rs and scripts/download-models.ts now name packages/desktop-app/app-lib/src/bundled_model.rs as the third copy of the digest. The config.rs comment also says the app's tests fail on a mismatch.

✅ Docs D1 🟢: the app reads NODESPACE_HOME from its own environment

daemon-cli-lifecycle.md now says the rule runs in the app's environment. On macOS, the launchd-registered daemon doesn't inherit NODESPACE_HOME, so an app launched with it set copies under that home while its daemon still looks under ~/.nodespace/.

• The copy steps also now read in the order they run: flush, check, rename.

✅ Docs D2 Nit: ADR-045 amendment

The amendment now credits ADR-058 with the guarantee that every install runs the same pinned file: the daemon refuses any file that fails the digest. The copy alone doesn't guarantee it.

Follow-ups

• release.yml checks no digest when it downloads the model.
• The app's daemon management ignores NODESPACE_HOME.

Both are listed for the coordinator to file at merge, as the review suggested. Neither is filed from this PR.

Verification (head 22ecaa90)

• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo fmt --check (app-lib, nlp-engine): clean.
• Prettier on download-models.ts: clean.
• check-pro-boundary: clean. The push check passed.
• bundled_model tests: 14 of 14 passed.
• All 14 mutations re-run: each fails its target test(s), and every test fails under at least one mutation.
• Full isolated nextest run -p nodespace-app-lib, excluding the two Gemma chat-model files: 374 passed, 6 skipped. No test daemon was left running.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: S1.1 changed how the copy is made off macOS: a new code path (stream_to_temp) on the platform this PR could not run on. It is small, unit-tested and mutation-checked, but it is new logic, so a quick re-review of 22ecaa90 alone is warranted. The other changes are comments and docs.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Re-Review — PR #3611 (round 2)

Review Type: Re-Review

Scope. Only the commits since round 1:

• Core 3d392ecc..22ecaa90: bundled_model.rs, plus the rotation comments in nlp-engine/src/config.rs and scripts/download-models.ts.
• Docs NodeSpaceAI/nodespace-docs#103, 131e153..6c71f66.

Both are confirmed as the PRs' current heads. I reviewed from the code; nothing was built or run, per the shared-machine rules.

Previous Review Summary

• Previous recommendations: 6 optional findings (S1–S4, D1, D2). S1 had two parts, so 7 items in all, plus 2 follow-ups to file.
• Addressed (5): S1.1, S3, S4, D1, D2.
• Skipped with justification (2), both accepted: S1.2, S2.
• New issues: none blocking; one optional 🟢 test suggestion (T1).

Item	Status	Verification
S1.1 Windows read-only attribute	✅ Fixed	stream_to_temp creates the file itself (create_new) and calls sync_all on that same write handle. No attribute is carried over, and FlushFileBuffers gets the GENERIC_WRITE handle it needs. The old sync_file reopen is gone. Side benefit: on a failed copy, fs::remove_file(&temp) no longer meets a read-only temp file on Windows, which could have left a 146 MB file behind.
S1.2 rename retry	⏭️ Skip accepted	A failure is non-fatal, retried on the next launch, and has never been observed. It is now named in the PR's "Not verified" section, with the log line to watch for. A retry loop for a failure nobody has seen would be untested code.
S2 hard_link no-clobber	⏭️ Skip accepted	On filesystems without hard links (exFAT/FAT32, some network shares), a harmless microsecond race would become a failure on every launch. That trade-off alone justifies the skip. commit's doc now states the window, and the PR body matches. The docs still say "never replaces". Given a microsecond window whose only realistic writer puts down the same verified bytes, that simplification is fair.
S3 pid comment	✅ Fixed	temp_path's doc now matches what remove_stale_temp_files does. Leaving the crash-orphan case alone is reasonable.
S4 rotation comments	✅ Fixed	Both comments now name bundled_model.rs.
D1 app environment / launchd	✅ Fixed	Checked: write_plist passes no NODESPACE_HOME. The Windows path starts the daemon with Command::new, so there it inherits the environment. Scoping the caveat to macOS is right.
D2 ADR-045 wording	✅ Fixed	Credits ADR-058's load-time refusal.

The steps now run copy → flush → hash → rename. That changes nothing for correctness: the hash reads the page cache either way. The module doc, the copy_verified and write_verified docs, and the architecture doc all describe that same order.

Code Review Findings (new commits only)

No 🔴 or 🟡 findings.

🟢 T1: Nothing tests the macOS copy from a read-only bundle
📁 packages/desktop-app/app-lib/src/bundled_model.rs:237-244, :419-439

• Problem. The new test calls stream_to_temp directly, and it does catch M14. On macOS and Linux it fails at the readonly() assertion, even when run as root. On Windows, CopyFileExW would carry the read-only attribute and fail it the same way.

  clone_to_temp is the path that actually runs on macOS. Its doc says the clone keeps the bundle's mode, "which may be read-only", so it flushes through a read-only descriptor. No test makes the bundle read-only on that path. Suppose someone later changes clone_to_temp to reopen the temp file for writing, to match the Windows path. Every macOS launch from a read-only bundle would then fail, and every test would still pass.

• Fix (optional, a few lines). Add one test that makes the bundle read-only, runs provision, and asserts Copied and the bytes. It runs whichever copy the platform uses: clone_to_temp on macOS, and the full real path on any other OS that runs the suite. Keep the existing direct test for the Windows path.

• Principle. Test the property each path's doc comment claims, on the path that ships.

Checked, no issues:

• stream_to_temp:
  • create_new is safe because the stale-file cleanup has already removed this pid's name. It also never follows a link left at the temp path.
  • If the copy stops early, the empty or partial temp file is removed by copy_verified's !matches!(…, Ok(Copied)) guard.
  • The write handle is dropped before the hash and the rename, so nothing holds the file open at rename time on Windows.
  • On Windows, io::copy moves 8 KiB at a time, which is negligible for a one-off 146 MB copy next to the hash.
• The cfg! split:
  • Both helpers are referenced on every platform, so there is no dead-code lint, and every build type-checks both.
  • Keeping fs::copy on macOS keeps the APFS clone's disk saving. That justifies two small helpers over streaming everywhere.
• The new test:
  • The read-only bundle doesn't break TempDir cleanup: on Unix, removing a file needs only a writable directory, and the drop ignores errors anyway.
  • The readonly() assertion checks mode bits, so it still catches the mutation when run as root.
• Comments and wording:
  • config.rs's "the app's tests fail on a mismatch" is accurate for the app ↔ nlp-engine pair. download-models.ts is still guarded only by a comment, as it was before this PR.
  • Neither diff has #NNNN references or Pro/sync wording.

Docs PR #103 (6c71f66)

Both edits are accurate and consistent with the code:

• the order of the copy steps matches the code;
• the D1 caveat is correctly limited to macOS;
• the D2 wording is right;
• the nested bullet matches the surrounding list's format.

Recommendation

APPROVE

• S1.1 is fixed properly, with a test that actually catches the regression.
• Both skips are sound and documented where a future reader will find them.
• T1 is the only new note, and it is optional.
• Follow-ups: the two out-of-scope issues still need filing at merge, as the author's response says: release.yml checks no digest when it downloads the model, and the app's daemon management ignores NODESPACE_HOME.

🤖 Generated with Claude Code

[mstomar125]

Automated pragmatic-code-review (round 2): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the re-review comment above.

[mstomar125]

Address review: response to the re-review (round 2)

Round 2 was APPROVE, with one optional finding. It is addressed in 41da40c8.

✅ T1 🟢: the macOS copy path from a read-only bundle

New test a_read_only_bundle_is_copied. It makes the placeholder bundle read-only, runs provision, and asserts Copied and the bytes.

• On macOS it goes through clone_to_temp, the path that ships. The clone keeps the read-only mode, so this pins the claim in clone_to_temp's doc that it flushes without opening for writing.
• The direct stream_to_temp test for the Windows path stays.
• Mutation M15: clone_to_temp reopens the temp file for writing. Only the new test fails.

Verification (head 41da40c8)

• bundled_model tests: 15 of 15 passed.
• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo fmt --check: clean. The push check passed.
• The PR body's test and mutation tables are updated.

Follow-ups (unchanged)

To file at merge:

• release.yml checks no digest when it downloads the model.
• The app's daemon management ignores NODESPACE_HOME.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: This round adds one test, which follows the reviewer's own suggested shape and is mutation-checked. No production code changed. Both review rounds came back APPROVE, and round 2 had no 🔴 or 🟡 findings.

🤖 Generated with Claude Code