Fix Mac workspace sync for guest symlinks - #45
Merged
Conversation
Signed-off-by: Joseph Yaksich <gitcommit90@users.noreply.github.com>
📝 WalkthroughWalkthroughThe release updates version and channel-machine image references to ChangesChannel image and workspace export
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/server/channel-computers.ts`:
- Around line 1036-1043: Update the export flow around isolatedInvocation and
validateMirrorTree so symlink exclusion is atomic with archiving: use a
read-only snapshot of /workspace or quiesce guest writes before generating and
archiving the file list, ensuring tar cannot follow a path replaced after find
emits it. Preserve ordinary directory/file export semantics, and add a
regression test covering replacement of an emitted directory with a symlink
during export.
In `@src/server/db.ts`:
- Line 942: Update the channel-computer provisioning flow in src/server/db.ts at
lines 942-942 and src/server/channel-computers.ts at lines 71-71 so existing
rows using image 0.0.31 are migrated to 0.0.32, and explicitly replace or
reprovision their machines rather than only restarting them; preserve INSERT OR
IGNORE behavior for unaffected records and ensure ensureAppleProvisioned uses
the updated image.
In `@test/channel-computers.mjs`:
- Line 108: Update the test around the guest workspace Python path to import and
use lstatSync from node:fs instead of existsSync, so the assertion inspects the
directory entry itself without following a potentially dangling symlink and
verifies that the entry is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27f9dfa8-0f74-4ecf-8d18-67741d8a7ddc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdpackage.jsonsrc/server/channel-computers.tssrc/server/db.tstest/channel-computers.mjstest/fake-container.mjs
Comment on lines
+1036
to
+1043
| // Guest workspaces legitimately contain symlinks (Python venvs are the | ||
| // common case). The host mirror deliberately has no symlink semantics, | ||
| // so export only ordinary directories/files. Symlinks remain durable in | ||
| // the guest VM and can never be followed across the host boundary. | ||
| const invocation = isolatedInvocation([ | ||
| "/bin/bash", "-lc", | ||
| "set -o pipefail; cd /; find -P workspace -xdev \\( -type d -o -type f \\) -print0 | tar --null --no-recursion -T - -cf -", | ||
| ], computer, "agent", "/workspace"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/workspace" "$tmp/outside"
printf '%s\n' outside-secret > "$tmp/outside/secret"
ln -s "$tmp/outside" "$tmp/workspace/ancestor"
printf '%s\0' 'workspace/ancestor/secret' |
(cd "$tmp" && tar --null --no-recursion -T - -cf "$tmp/archive.tar") \
2>"$tmp/tar.err" || true
if tar -xOf "$tmp/archive.tar" 'workspace/ancestor/secret' 2>/dev/null |
grep -Fxq outside-secret; then
echo "FAIL: tar follows a symlink ancestor" >&2
exit 1
fi
echo "PASS: tar did not archive through the symlink ancestor"Repository: gitcommit90/1Helm
Length of output: 192
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target code ---'
sed -n '1000,1075p' src/server/channel-computers.ts
printf '%s\n' '--- validation and export references ---'
rg -n -C 4 'validateMirrorTree|isolatedInvocation|--no-recursion|find -P|mirror' src/server/channel-computers.ts src test tests 2>/dev/null || true
printf '%s\n' '--- tar implementation ---'
tar --version | head -n 2Repository: gitcommit90/1Helm
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/workspace/ancestor" "$tmp/outside"
printf '%s\n' guest-secret >"$tmp/workspace/ancestor/secret"
printf '%s\n' outside-secret >"$tmp/outside/secret"
(
cd "$tmp"
find -P workspace -xdev \( -type d -o -type f \) -print0 >"$tmp/entries"
)
# Model the TOCTOU window between find's path emission and tar's path lookup.
mv "$tmp/workspace/ancestor" "$tmp/workspace/ancestor.real"
ln -s "$tmp/outside" "$tmp/workspace/ancestor"
(
cd "$tmp"
tar --null --no-recursion -T "$tmp/entries" -cf "$tmp/archive.tar"
)
if tar -xOf "$tmp/archive.tar" 'workspace/ancestor/secret' 2>/dev/null |
grep -Fxq outside-secret; then
echo "FAIL: the exact find/tar sequence archived data through a replaced ancestor"
exit 1
fi
echo "PASS: the exact find/tar sequence did not archive outside data"Repository: gitcommit90/1Helm
Length of output: 231
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- synchronization and guest-write context ---'
sed -n '740,885p' src/server/channel-computers.ts
printf '%s\n' '--- terminal and command call sites ---'
sed -n '885,970p' src/server/channel-computers.tsRepository: gitcommit90/1Helm
Length of output: 15962
Make symlink exclusion atomic with the archive.
find -P passes paths to a separate tar process while active guest commands and terminals can modify /workspace. If a guest process replaces an emitted directory with a symlink, tar --no-recursion -T can archive data outside /workspace. validateMirrorTree cannot detect the source inode that tar read.
Use a read-only snapshot or quiesce guest writes during export. Add a race regression test.
🤖 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 `@src/server/channel-computers.ts` around lines 1036 - 1043, Update the export
flow around isolatedInvocation and validateMirrorTree so symlink exclusion is
atomic with archiving: use a read-only snapshot of /workspace or quiesce guest
writes before generating and archiving the file list, ensuring tar cannot follow
a path replaced after find emits it. Preserve ordinary directory/file export
semantics, and add a regression test covering replacement of an emitted
directory with a symlink during export.
| const configuredBackend = String(process.env.HELM_CHANNEL_COMPUTER_BACKEND || platformBackend); | ||
| const backend = ["apple", "oci", "native", "mock"].includes(configuredBackend) ? configuredBackend : platformBackend; | ||
| const image = String(process.env.HELM_CHANNEL_MACHINE_IMAGE || "local/1helm-channel-machine:0.0.31"); | ||
| const image = String(process.env.HELM_CHANNEL_MACHINE_IMAGE || "local/1helm-channel-machine:0.0.32"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'DEFAULT_CHANNEL_IMAGE|HELM_CHANNEL_MACHINE_IMAGE|INSERT OR IGNORE INTO channel_computers|UPDATE channel_computers.*image|reprovision|repair' \
src testRepository: gitcommit90/1Helm
Length of output: 38056
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- db.ts outline and initialization region ---'
ast-grep outline src/server/db.ts --match 'function $_($$$) { $$$ }' --view summary | head -200
sed -n '700,780p' src/server/db.ts
sed -n '900,980p' src/server/db.ts
printf '%s\n' '--- channel-computers.ts outline ---'
ast-grep outline src/server/channel-computers.ts --match 'function $_($$$) { $$$ }' --view summary | head -260
printf '%s\n' '--- candidate lifecycle and migration code ---'
rg -n -C 8 \
'channelComputer\(|ensureChannelComputer|reconcileChannelComputers|provision_status|computer\.image|image: computer|image\)|CREATE TABLE.*channel_computers|ALTER TABLE.*channel_computers|PRAGMA user_version|schema|migration' \
src/server/channel-computers.ts src/server/db.ts src/server/*.tsRepository: gitcommit90/1Helm
Length of output: 354
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- db.ts initialization region ---'
sed -n '700,780p' src/server/db.ts
sed -n '900,980p' src/server/db.ts
printf '%s\n' '--- channel-computers.ts declarations ---'
ast-grep outline src/server/channel-computers.ts --view signatures | head -260
printf '%s\n' '--- lifecycle and migration references ---'
rg -n -C 8 \
'channelComputer\(|ensureChannelComputer|reconcileChannelComputers|provision_status|computer\.image|image: computer|CREATE TABLE.*channel_computers|ALTER TABLE.*channel_computers|PRAGMA user_version|migration|schema' \
src/server/channel-computers.ts src/server/db.ts src/server/*.tsRepository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- record creation and provisioning ---'
sed -n '120,230p' src/server/channel-computers.ts
sed -n '583,805p' src/server/channel-computers.ts
printf '%s\n' '--- reconciliation and update paths ---'
sed -n '1310,1515p' src/server/channel-computers.ts
printf '%s\n' '--- all channel_computers image writes and image consumers ---'
rg -n -C 3 \
'channel_computers.*image|SET[^"\n]*image|image[^"\n]*channel_computers|ensureAppleProvisioned|ensureOciProvisioned|provisionChannelComputer|reconcileOne' \
src/server/db.ts src/server/channel-computers.tsRepository: gitcommit90/1Helm
Length of output: 43491
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import sqlite3
db = sqlite3.connect(":memory:")
db.executescript("""
CREATE TABLE channel_computers (
channel_id INTEGER PRIMARY KEY,
image TEXT NOT NULL
);
INSERT INTO channel_computers VALUES (7, 'local/1helm-channel-machine:0.0.31');
""")
db.execute(
"""INSERT OR IGNORE INTO channel_computers(channel_id, image)
VALUES (?, ?)""",
(7, "local/1helm-channel-machine:0.0.32"),
)
row = db.execute(
"SELECT channel_id, image FROM channel_computers WHERE channel_id=7"
).fetchone()
print({"after_insert_or_ignore": row})
db.execute(
"UPDATE channel_computers SET image=? WHERE channel_id=?",
("local/1helm-channel-machine:0.0.32", 7),
)
row = db.execute(
"SELECT channel_id, image FROM channel_computers WHERE channel_id=7"
).fetchone()
print({"after_explicit_update": row})
PY
printf '%s\n' '--- image versions and machine replacement controls ---'
rg -n -C 3 \
'0\.0\.3[01-9]|image-contract|machine (create|delete|rm|stop|run)|container (create|rm|stop|start)|DELETE FROM channel_computers|UPDATE channel_computers SET[^;]*image' \
src container scripts testRepository: gitcommit90/1Helm
Length of output: 17475
Migrate existing channel computers to image 0.0.32.
INSERT OR IGNORE preserves existing channel_computers.image values. ensureAppleProvisioned uses the stored image only when it creates a missing machine. Update existing rows and explicitly replace or reprovision machines that still use 0.0.31; restarting them does not load the new image.
📍 Affects 2 files
src/server/db.ts#L942-L942(this comment)src/server/channel-computers.ts#L71-L71
🤖 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 `@src/server/db.ts` at line 942, Update the channel-computer provisioning flow
in src/server/db.ts at lines 942-942 and src/server/channel-computers.ts at
lines 71-71 so existing rows using image 0.0.31 are migrated to 0.0.32, and
explicitly replace or reprovision their machines rather than only restarting
them; preserve INSERT OR IGNORE behavior for unaffected records and ensure
ensureAppleProvisioned uses the updated image.
| result = await computers.runChannelCommand(alpha.channelId, "mkdir -p .venv/bin; ln -s /usr/bin/python3 .venv/bin/python3; ln -s python3 .venv/bin/python; printf venv-ok > .venv/marker"); | ||
| assert.equal(result.exit_code, 0, "a normal Python-style venv does not make the command fail during mirror sync"); | ||
| assert.equal(readFileSync(join(dataDir, "channels", String(alpha.channelId), "workspace", ".venv", "marker"), "utf8"), "venv-ok"); | ||
| assert.equal(existsSync(join(dataDir, "channels", String(alpha.channelId), "workspace", ".venv", "bin", "python3")), false, "guest symlinks stay in the VM instead of crossing the host mirror boundary"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'NODE'
const { existsSync, lstatSync, mkdtempSync, rmSync, symlinkSync } = require("node:fs");
const { join } = require("node:path");
const { tmpdir } = require("node:os");
const dir = mkdtempSync(join(tmpdir(), "symlink-test-"));
const link = join(dir, "python3");
try {
symlinkSync("/definitely-missing-python3", link);
if (existsSync(link)) throw new Error("existsSync unexpectedly returned true");
if (!lstatSync(link).isSymbolicLink()) throw new Error("lstatSync did not detect the symlink");
} finally {
rmSync(dir, { recursive: true, force: true });
}
NODERepository: gitcommit90/1Helm
Length of output: 155
🏁 Script executed:
sed -n '1,125p' test/channel-computers.mjsRepository: gitcommit90/1Helm
Length of output: 10019
🏁 Script executed:
rg -n -C 5 'symlink|mirror|lstat|existsSync|workspace' src test/fake-container.mjsRepository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
fd -i 'channel-computers|fake-container' src testRepository: gitcommit90/1Helm
Length of output: 324
🏁 Script executed:
rg -n -C 8 'sync|mirror|symlink|workspace|files' src/server/channel-computers.ts test/fake-container.mjsRepository: gitcommit90/1Helm
Length of output: 50374
🏁 Script executed:
sed -n '808,980p' src/server/channel-computers.tsRepository: gitcommit90/1Helm
Length of output: 11571
🏁 Script executed:
rg -n -A 90 'function syncGuestToHostUnlocked|validateMirrorTree' src/server/channel-computers.tsRepository: gitcommit90/1Helm
Length of output: 18663
Inspect the directory entry instead of following the symlink.
existsSync follows symlinks and returns false for a dangling symlink. Use lstatSync and assert that the entry itself is absent. Import lstatSync from node:fs.
🤖 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 `@test/channel-computers.mjs` at line 108, Update the test around the guest
workspace Python path to import and use lstatSync from node:fs instead of
existsSync, so the assertion inspects the directory entry itself without
following a potentially dangling symlink and verifies that the entry is absent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the release-blocking macOS channel failure where ordinary guest symlinks, especially Python virtual environments, caused command completion, stop, and archive to fail during guest-to-host mirror sync. Guest symlinks now remain durable inside the isolated Linux computer and are omitted from the deliberately symlink-free host mirror. Adds a focused Python-style venv regression and bumps the candidate to 0.0.32.\n\nVerification: npm run typecheck; node --test test/channel-computers.mjs test/desktop.mjs; git diff --check.\n\nSigned-off-by: Joseph Yaksich gitcommit90@users.noreply.github.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation