Restore fresh runtime setup and resident outcomes - #52
Merged
Conversation
📝 WalkthroughWalkthroughRelease 0.0.35 updates Windows setup status handling, nested OCI networking and readiness checks, crun AppArmor management, resident-agent escalation behavior, and channel-machine version references. ChangesRuntime and Setup Release
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/windows-wsl-status.ps1 (1)
1-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExecute
test/windows-wsl-status.ps1in CI.
npm testonly checks this file as text throughtest/desktop.mjs;.github/workflows/ci.ymlhas no PowerShell invocation. Thethrowassertions do not run automatically.🤖 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/windows-wsl-status.ps1` around lines 1 - 42, Add an explicit CI step in the existing workflow to execute test/windows-wsl-status.ps1 with PowerShell on a compatible Windows runner. Ensure the step runs as part of the normal test job or workflow so its throw-based assertions execute, while preserving the existing desktop.mjs text-check behavior.
🧹 Nitpick comments (1)
test/autonomy-platform.mjs (1)
151-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert all three mandatory playbooks.
Line 151 verifies only
outcome-ownership. Add assertions for non-emptyblocker-resolutionandskipper-escalationplaybooks. A regression that removes either required playbook currently passes this 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 `@test/autonomy-platform.mjs` around lines 151 - 154, Extend the assertions for first.context in the autonomy-platform test to verify that non-empty blocker-resolution and skipper-escalation playbooks are present, alongside the existing outcome-ownership assertion. Keep the current ordering/content checks and rejection assertion unchanged.
🤖 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 `@scripts/1helm-oci-runtime`:
- Around line 718-732: Wrap the ownership/storage health-check command in the
retry loop around the podman exec invocation, using the existing timeout
dependency and the same timeout duration as the later socket/DNS/TCP probe.
Preserve the current arguments, probe script, retry behavior, and success
handling in verify_container.
- Around line 703-704: Update the container creation flow around channel_root
and network_ready to remove or reset the retained network-ready-v1 marker before
the new container proceeds, including restore handling as applicable. Ensure
newly created containers cannot skip the network health check in the logic
around the create path.
In `@site/public/install-oci-runtime.sh`:
- Line 44: Update the package installation list and prerequisite validation in
the OCI setup script: include the apparmor package in the apt-get install set,
and ensure both prerequisite checks include apparmor_parser before the nested
AppArmor configuration runs. Preserve the existing failure behavior when the
parser remains unavailable.
In `@site/public/install.sh`:
- Line 17: Update rollback handling in site/public/install.sh (line 17),
site/public/apply-linux-release.sh (line 23), and site/public/update-host.sh
(line 25) to reload /etc/apparmor.d/crun after restoring host-contract files,
but only when apparmor_parser and the parent AppArmor profile are available;
retain the existing daemon-reload behavior.
---
Outside diff comments:
In `@test/windows-wsl-status.ps1`:
- Around line 1-42: Add an explicit CI step in the existing workflow to execute
test/windows-wsl-status.ps1 with PowerShell on a compatible Windows runner.
Ensure the step runs as part of the normal test job or workflow so its
throw-based assertions execute, while preserving the existing desktop.mjs
text-check behavior.
---
Nitpick comments:
In `@test/autonomy-platform.mjs`:
- Around line 151-154: Extend the assertions for first.context in the
autonomy-platform test to verify that non-empty blocker-resolution and
skipper-escalation playbooks are present, alongside the existing
outcome-ownership assertion. Keep the current ordering/content checks and
rejection assertion unchanged.
🪄 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: fb346c23-a77d-4298-a496-697eadf9313d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
CHANGELOG.mdREADME.mdpackage.jsonscripts/1helm-oci-runtimescripts/install-wsl-runtime.ps1site/public/apply-linux-release.shsite/public/install-oci-runtime.shsite/public/install.shsite/public/uninstall-host.shsite/public/update-host.shsrc/server/bots.tssrc/server/channel-computers.tssrc/server/db.tssrc/server/skills.tstest/autonomy-platform.mjstest/channel-computers.mjstest/desktop.mjstest/mock-openai.mjstest/native-world.mjstest/site.mjstest/windows-wsl-status.ps1
Comment on lines
+703
to
+704
| root="$(channel_root "$name")" | ||
| network_ready="$root/network-ready-v1" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether container/channel recreation clears the network-ready-v1 marker.
rg -n 'network-ready-v1|channel_root\(' scripts/1helm-oci-runtimeRepository: gitcommit90/1Helm
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant function map ---'
ast-grep outline scripts/1helm-oci-runtime --match '$_' --view compact 2>/dev/null | sed -n '1,220p'
printf '%s\n' '--- marker and lifecycle references ---'
rg -n -C 8 'network_ready|network-ready-v1|create_container|destroy_container|remove_container|CHANNELS_ROOT|channel_root' scripts/1helm-oci-runtime
printf '%s\n' '--- lifecycle section ---'
sed -n '1,130p' scripts/1helm-oci-runtime
sed -n '650,770p' scripts/1helm-oci-runtimeRepository: gitcommit90/1Helm
Length of output: 30242
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- definitions and callers ---'
rg -n -C 12 '^(channel_root|create_container|delete_container|destroy_container|recreate_container|cleanup|remove|reset|main)[[:space:]]*\(\)|channel_root|create_container|network_ready|network-ready-v1|CHANNELS_ROOT' scripts/1helm-oci-runtime
printf '%s\n' '--- lines 680-760 ---'
sed -n '680,760p' scripts/1helm-oci-runtimeRepository: gitcommit90/1Helm
Length of output: 26319
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("scripts/1helm-oci-runtime").read_text()
marker = 'network-ready-v1'
def section(start, end):
a = source.index(start)
b = source.index(end, a)
return source[a:b]
create = section(' create)\n', ' inspect)\n')
delete = section(' delete)\n', ' # Host Files/Cowork bridge')
restore = section('restore_container() {', '\noperation=')
checks = {
"delete removes channel root": 'rm -rf -- "$root"' in delete,
"create invokes prepare_storage before create_container": create.index('prepare_storage') < create.index('create_container'),
"create does not remove network-ready-v1": marker not in create,
"restore imports channel tree and starts container": 'mv -T "$temporary/channel" "$root"' in restore and 'start_container "$name" "$owner"' in restore,
"restore does not remove network-ready-v1": marker not in restore,
}
for name, result in checks.items():
print(f"{name}: {'YES' if result else 'NO'}")
PYRepository: gitcommit90/1Helm
Length of output: 394
Clear network-ready-v1 when creating a new container from retained channel storage. The create and restore paths preserve the marker, so a new container can skip the network health check at lines 734-745.
🤖 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 `@scripts/1helm-oci-runtime` around lines 703 - 704, Update the container
creation flow around channel_root and network_ready to remove or reset the
retained network-ready-v1 marker before the new container proceeds, including
restore handling as applicable. Ensure newly created containers cannot skip the
network health check in the logic around the create path.
Comment on lines
718
to
732
| for _ in {1..100}; do | ||
| # Podman can briefly fail name-based --user lookup while it reconstructs | ||
| # its ephemeral runroot after a host reboot even though the persisted | ||
| # image passwd database is intact. These identities are pinned by the | ||
| # installed runtime manifest and image contract, so use their numeric form | ||
| # for the readiness probe and every subsequent exec boundary. | ||
| if "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c 'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' 1helm-start "$owner" >/dev/null 2>&1; then | ||
| if "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c \ | ||
| 'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' \ | ||
| 1helm-start "$owner" >/dev/null 2>&1; then | ||
| verify_container "$name" "$owner" | ||
| return | ||
| container_ok=1 | ||
| break | ||
| fi | ||
| sleep 0.1 | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap the ownership/storage health-check exec in timeout.
This loop retries "${PODMAN[@]}" exec --user 0:0 "$name" ... up to 100 times, but the exec call itself has no timeout. If a single podman exec invocation hangs (for example, a container whose init process is unresponsive after a host reboot, which the surrounding comment explicitly anticipates), the loop blocks on that call indefinitely instead of retrying, and the script never reaches die. The socket/DNS/TCP probe added a few lines later (Line 736) is correctly wrapped in timeout 7, and timeout is added as a required dependency at Line 956 specifically for this function — apply the same protection here for consistency and to guarantee the retry loop cannot hang.
🐛 Proposed fix to bound the ownership/storage probe
for _ in {1..100}; do
- if "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c \
+ if timeout 7 "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c \
'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' \
1helm-start "$owner" >/dev/null 2>&1; then📝 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.
Suggested change
| for _ in {1..100}; do | |
| # Podman can briefly fail name-based --user lookup while it reconstructs | |
| # its ephemeral runroot after a host reboot even though the persisted | |
| # image passwd database is intact. These identities are pinned by the | |
| # installed runtime manifest and image contract, so use their numeric form | |
| # for the readiness probe and every subsequent exec boundary. | |
| if "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c 'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' 1helm-start "$owner" >/dev/null 2>&1; then | |
| if "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c \ | |
| 'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' \ | |
| 1helm-start "$owner" >/dev/null 2>&1; then | |
| verify_container "$name" "$owner" | |
| return | |
| container_ok=1 | |
| break | |
| fi | |
| sleep 0.1 | |
| done | |
| for _ in {1..100}; do | |
| # Podman can briefly fail name-based --user lookup while it reconstructs | |
| # its ephemeral runroot after a host reboot even though the persisted | |
| # image passwd database is intact. These identities are pinned by the | |
| # installed runtime manifest and image contract, so use their numeric form | |
| # for the readiness probe and every subsequent exec boundary. | |
| if timeout 7 "${PODMAN[@]}" exec --user 0:0 "$name" /bin/sh -c \ | |
| 'test "$(cat /var/lib/1helm/owner)" = "$1" && test -d /workspace && test -d /home/agent' \ | |
| 1helm-start "$owner" >/dev/null 2>&1; then | |
| verify_container "$name" "$owner" | |
| container_ok=1 | |
| break | |
| fi | |
| sleep 0.1 | |
| done |
🤖 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 `@scripts/1helm-oci-runtime` around lines 718 - 732, Wrap the ownership/storage
health-check command in the retry loop around the podman exec invocation, using
the existing timeout dependency and the same timeout duration as the later
socket/DNS/TCP probe. Preserve the current arguments, probe script, retry
behavior, and success handling in verify_container.
| container_virt="$(systemd-detect-virt --container 2>/dev/null || true)" | ||
| apparmor_enabled="$(cat /sys/module/apparmor/parameters/enabled 2>/dev/null || true)" | ||
| if [[ -n "$container_virt" && "$container_virt" != none && "$apparmor_enabled" =~ ^[Yy]$ && -r /etc/apparmor.d/crun ]]; then | ||
| command -v apparmor_parser >/dev/null || { echo "Nested AppArmor OCI setup requires apparmor_parser." >&2; exit 1; } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Confirm that the installer provisions the command it requires.
grep -nE 'apt-get install.*apparmor|for command in .*apparmor_parser' \
site/public/install-oci-runtime.sh
# Run on each supported Ubuntu/Debian base image.
command -v apparmor_parserRepository: gitcommit90/1Helm
Length of output: 155
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- installer context ---'
sed -n '1,90p' site/public/install-oci-runtime.sh
printf '%s\n' '--- AppArmor references ---'
rg -n -C 3 'apparmor|apt-get install|command -v' site/public/install-oci-runtime.shRepository: gitcommit90/1Helm
Length of output: 8609
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("site/public/install-oci-runtime.sh")
text = path.read_text()
install_lines = [
line.strip() for line in text.splitlines()
if "apt-get install" in line
]
command_loops = [
line.strip() for line in text.splitlines()
if re.search(r"for command in", line)
]
print("apt install lines:")
print("\n".join(install_lines))
print("command prerequisite loops:")
print("\n".join(command_loops))
print("apparmor in install set:", any(re.search(r"\bapparmor\b", line) for line in install_lines))
print("apparmor_parser in prerequisite loops:",
any("apparmor_parser" in line for line in command_loops))
PYRepository: gitcommit90/1Helm
Length of output: 826
🌐 Web query:
Debian Ubuntu package apparmor_parser provided by apparmor package official package contents
💡 Result:
In both Debian and Ubuntu, the apparmor_parser executable is officially provided by the apparmor binary package [1][2][3]. The apparmor package is the core user-space utility package for AppArmor, which includes the apparmor_parser tool used to compile and load AppArmor security policies into the Linux kernel [1][4][3]. Depending on the specific system configuration and version history, this file may be located in /usr/sbin/apparmor_parser or historically in /sbin/apparmor_parser [3][5][6][7]. Modern distributions typically utilize a merged /usr layout where these paths are unified [7].
Citations:
- 1: https://packages.debian.org/source/unstable/apparmor
- 2: https://packages.ubuntu.com/en/source/jammy/apparmor
- 3: https://packages.debian.org/sid/amd64/apparmor/filelist
- 4: https://manpages.debian.org/trixie/apparmor/apparmor_parser.8.en.html
- 5: https://aptpeek.dev/ubuntu/focal-security/main/amd64/apparmor/2.13.3-7ubuntu5.4
- 6: https://aptpeek.dev/ubuntu/questing-updates/main/amd64/apparmor/5.0.0~alpha1-0ubuntu8.3
- 7: https://bugs.launchpad.net/bugs/2017594
Install the AppArmor parser before requiring it.
Add apparmor to the apt-get install set and add apparmor_parser to both prerequisite checks. Otherwise, a nested host can skip installation and exit when the parser check runs.
🤖 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 `@site/public/install-oci-runtime.sh` at line 44, Update the package
installation list and prerequisite validation in the OCI setup script: include
the apparmor package in the apt-get install set, and ensure both prerequisite
checks include apparmor_parser before the nested AppArmor configuration runs.
Preserve the existing failure behavior when the parser remains unavailable.
| /usr/libexec/1helm-oci-runtime | ||
| /etc/1helm/oci-runtime-v1.conf | ||
| /etc/sudoers.d/1helm-oci-runtime | ||
| /etc/apparmor.d/local/crun |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reload the crun AppArmor profile during rollback.
A failed transaction can reload the new local profile before rollback. Each rollback restores /etc/apparmor.d/local/crun on disk, but only runs systemctl daemon-reload. That command does not reload AppArmor policy. The kernel can retain the new socket grants after rollback.
site/public/install.sh#L17-L17: After restoring host-contract files, reload/etc/apparmor.d/crunwhenapparmor_parserand the parent profile are available.site/public/apply-linux-release.sh#L23-L23: After restoring host-contract files, reload/etc/apparmor.d/crunwhenapparmor_parserand the parent profile are available.site/public/update-host.sh#L25-L25: After restoring host-contract files, reload/etc/apparmor.d/crunwhenapparmor_parserand the parent profile are available.
📍 Affects 3 files
site/public/install.sh#L17-L17(this comment)site/public/apply-linux-release.sh#L23-L23site/public/update-host.sh#L25-L25
🤖 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 `@site/public/install.sh` at line 17, Update rollback handling in
site/public/install.sh (line 17), site/public/apply-linux-release.sh (line 23),
and site/public/update-host.sh (line 25) to reload /etc/apparmor.d/crun after
restoring host-contract files, but only when apparmor_parser and the parent
AppArmor profile are available; retain the existing daemon-reload behavior.
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.
Outcome
restart_requiredstate before signed-in WSL probingVerification
npm run typechecknpm run buildnpm run test:onboarding-browser(20/20)npm test(zero failures; two browser checks skipped because local Chrome is absent)npm run test:native(127/127)git diff --checktest/windows-wsl-status.ps1Summary by CodeRabbit
Bug Fixes
Release