fix(sandbox): bundle pinned Sealos skills for offline preparation - #154
Conversation
|
@cursoragent review this |
norberia
left a comment
norberia
left a comment
There was a problem hiding this comment.
Request changes
Direction is right: pin the Sealos Skill bundle in the sandbox image, prepare offline, fail closed, no runtime npm/GitHub fallback. I reproduced the 8 Node tests, git fetch --depth 1 origin bdd824cf… against GitHub, and a real plugins/sealos build (3 skills, 13 files, prepare twice with identical digest).
Do not pair this with Brain until the items below land, and a sandbox base then runtime image is actually built and smoked. Node tests on a fixture are not that gate. Companion Brain PR is out of scope here; the contract this image exports is /usr/local/bin/sealai-prepare-skills → /home/devbox/project/.agents/skills.
Must fix
- Kill/retry can drop unrelated Skills and still return
ready. Death between the tworenames leavesskills-backup-<oldpid>and no liveskills. The next prepare mkdir’s an empty tree, overlays only bundled names, and reports success. Restore leftovers before creatingskills;mkdtempoutside.agents. buildBundlecan emit a bundleverifyBundlerejects ifdestinationis dirty. Wipe dest, then verify at end of build (and in the DockerfileRUN).- Only
sealos-deployis required. That skill depends on siblinguse-sealosandk8s-kaniko-job. Lock all three in build/verify/smoke. - CI never sees the real pin. Fetch the SHA and
build+verifyin this workflow. Then dispatch basesandbox/v1→ runtimesandbox/v1smoke before Brain (runtime-images/.../sandbox/v1stillFROM sandbox-v1:v0.0.1-alpha.1-…).
Same pass
- Do not SHA-256 workspace skill bodies in
prepareBundle(hashes unused; 30s cap). prepare-skills: absolutenode/flock; parse success JSON; flock wait vs Brain 30s; CLI main-guard needsrealpathor empty stdout is treated as success.- After the second rename,
rm(backup)is best-effort. - Do not mkdir
/home/devbox/projectif clone did not; require the workspace root. - Skills stage: drop unpinned Docker Hub
node:22-bookworm-slim; reuse GHCRnode.js-22or pin a digest.
Already good
Full-SHA fetch + rev-parse check, canonical plugins/sealos (top-level skills/ are aliases at this pin), verify-before-write, symlink rejection, preserve siblings and root skills-lock.json, fail-closed generic errors, honest “Docker/Devbox not run” note.
Rollout
Publish sandbox-v1 base, then runtime, record the digest. Point Brain at that digest and remove nonempty DEPLOY_SKILL_SOURCE in the same deploy. Roll the pair back together. Clone, then prepare — not the reverse.
| export async function prepareBundle(bundle, workspace) { | ||
| const manifest = await verifyBundle(bundle); | ||
| await safeDirectory(workspace); | ||
| const agentRoot = path.join(workspace, '.agents'); | ||
| await safeDirectory(agentRoot); | ||
| const target = path.join(agentRoot, 'skills'); | ||
| await safeDirectory(target); | ||
| await files(target); | ||
| const stage = path.join(agentRoot, `skills-stage-${process.pid}`); | ||
| const backup = path.join(agentRoot, `skills-backup-${process.pid}`); | ||
| // Exclusive mkdir avoids following paths placed by a repository. | ||
| await mkdir(stage); | ||
| let backedUp = false; | ||
| try { | ||
| await cp(target, stage, { recursive: true }); | ||
| for (const name of manifest.skills) { | ||
| await rm(path.join(stage, name), { recursive: true, force: true }); | ||
| await cp(path.join(bundle, 'skills', name), path.join(stage, name), { recursive: true }); | ||
| } | ||
| // Reserve backup name; refuse any existing entry. | ||
| await mkdir(backup); | ||
| await rename(target, backup); | ||
| backedUp = true; | ||
| await rename(stage, target); | ||
| backedUp = false; | ||
| await rm(backup, { recursive: true, force: true }); | ||
| } catch (error) { |
There was a problem hiding this comment.
prepareBundle creates skills if it is missing (safeDirectory), then overlays only manifest.skills.
If this process is killed between rename(target, backup) and rename(stage, target) (Brain’s 30s cap), the next call mkdir’s an empty skills, copies bundled names, and returns {status:"ready"}. Unrelated project Skills are left in skills-backup-<oldpid> and look like they were never there. Reproduced with leftover backup/stage dirs and no live skills.
That breaks the preserve-unrelated-Skills invariant: retry looks successful, so nobody inspects leftovers.
Before creating skills, restore skills-backup-* (prefer) or skills-stage-*; fail closed if both exist and differ. Keep stage/backup via mkdtemp outside .agents (not process.pid). After the second rename, rm(backup) must be best-effort — do not throw once target is in place.
| await files(directory); | ||
| names.push(entry); | ||
| } | ||
| if (!names.includes('sealos-deploy')) fail('missing_deploy_skill'); |
There was a problem hiding this comment.
This only requires sealos-deploy. At pin bdd824cf that skill is a single SKILL.md whose implementation is ../use-sealos/ and ../k8s-kaniko-job/. A later revision can drop either and still produce a green image that fails managed deploy at runtime.
Require the sorted set k8s-kaniko-job, sealos-deploy, use-sealos in both buildBundle and verifyBundle. Smoke should assert skillCount == 3 and all three SKILL.md files.
| await mkdir(path.join(destination, 'skills'), { recursive: true }); | ||
| for (const name of names) await cp(path.join(sourceSkills, name), path.join(destination, 'skills', name), { recursive: true }); | ||
| const manifest = { schema: 1, revision, skills: names, files: await files(path.join(destination, 'skills')) }; | ||
| await writeFile(path.join(destination, 'manifest.json'), JSON.stringify(manifest)); |
There was a problem hiding this comment.
buildBundle does not wipe destination. Rebuilding into a dirty dir leaves extra skill trees in manifest.files while skills stays the current name list; verifyBundle then fails invalid_manifest. Reproduced: build exit 0, verify skill_bundle_unavailable.
Docker RUN on empty /bundle hides this. rm destination first (or only hash copied names) and call verifyBundle(destination) at the end of build — including in the Dockerfile RUN.
| await safeDirectory(agentRoot); | ||
| const target = path.join(agentRoot, 'skills'); | ||
| await safeDirectory(target); | ||
| await files(target); |
There was a problem hiding this comment.
files() SHA-256s every regular file. Here the return value is discarded — this is only a symlink/special-file scan of the workspace. A custom skill with node_modules or large blobs still pays full-content hashing under Brain’s 30s cap, then cps the same tree.
Split a lstat-only walker for prepare. Keep hashing for verifyBundle / buildBundle.
| async function safeDirectory(directory) { | ||
| const parent = path.dirname(directory); | ||
| if (parent !== directory) await safeDirectory(parent); | ||
| await mkdir(directory).catch(error => { if (error.code !== 'EEXIST') throw error; }); | ||
| if (!(await lstat(directory)).isDirectory()) fail('workspace_symlink'); | ||
| } | ||
|
|
||
| // Caller holds flock for the whole operation. Stage before replacing; preserve | ||
| // unrelated project skills and reject symlinks before any copy or rename. | ||
| export async function prepareBundle(bundle, workspace) { | ||
| const manifest = await verifyBundle(bundle); | ||
| await safeDirectory(workspace); |
There was a problem hiding this comment.
safeDirectory(workspace) mkdir’s /home/devbox/project if clone did not happen. Prepare then returns ready against an empty project.
lstat the workspace root and require an existing directory. Only create .agents / skills under it.
| if output=$(flock --wait 10 /tmp/sealai-runtime-skills.lock node /opt/sealai/skill-bundle.mjs prepare /opt/sealai/skill-bundle /home/devbox/project 2>/dev/null); then | ||
| printf '%s\n' "$output" | ||
| else | ||
| printf '%s\n' '{"schema":1,"status":"failed","reason":"skill_bundle_unavailable"}' >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Three wrapper holes:
- Unqualified
node/flockonPATH. After clone, a repo-fronted PATH can plant a binary that prints fakereadyJSON. Use/usr/bin/flockand the real Node path. 2>/dev/nullplus a catch-allskill_bundle_unavailablecollapses flock timeout, missing bundle, and CLI no-op. Require stdout to parse as{schema:1,status:"ready",...}or fail closed.--wait 10vs Brain’s 30s cap: a second concurrent prepare fails at 10s. Raise wait to just under the cap, or wait and let Brain kill.
Also: the CLI main-guard uses path.resolve(argv[1]) === fileURLToPath(import.meta.url) without realpath. Invoking via a symlink exits 0 with empty stdout; this wrapper then reports success.
| FROM node:22-bookworm-slim AS skills | ||
| ARG SEALOS_SKILLS_REPOSITORY=https://github.com/norberia/sealos-skills-next.git | ||
| ARG SEALOS_SKILLS_REVISION=bdd824cf2fd6c72896f8e201f32259cc8aed3f98 | ||
| RUN apt-get update && apt-get install -y --no-install-recommends git ca-certificates && rm -rf /var/lib/apt/lists/* | ||
| COPY skill-bundle.mjs /build/skill-bundle.mjs | ||
| RUN git init /source && git -C /source remote add origin "$SEALOS_SKILLS_REPOSITORY" && \ | ||
| git -C /source fetch --depth 1 origin "$SEALOS_SKILLS_REVISION" && \ | ||
| git -C /source checkout --detach FETCH_HEAD && \ | ||
| test "$(git -C /source rev-parse HEAD)" = "$SEALOS_SKILLS_REVISION" && \ | ||
| node /build/skill-bundle.mjs build /source/plugins/sealos /bundle "$SEALOS_SKILLS_REVISION" |
There was a problem hiding this comment.
Every other stage in this file is GHCR. This one pulls unpinned Docker Hub node:22-bookworm-slim (tag float + Hub rate limit + extra compromise path). The bundler Node never ships, but it still writes /bundle.
Reuse ${REGISTRY}/${REPO}/node.js-22:${NODE_IMAGE_VERSION} (git is already on that image) or pin a digest. git fetch --depth 1 origin $SHA against this pin does work; I ran the same sequence.
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '22' | ||
| - run: node --test base-images/frameworks/sandbox/v1/skill-bundle.test.mjs |
There was a problem hiding this comment.
This job never fetches the pinned source. The 8 tests use a one-skill fixture, so they cannot see the real description: > frontmatter, script tree, or top-level skills/ aliases.
Add a step: git fetch --depth 1 bdd824cf… → node skill-bundle.mjs build /source/plugins/sealos … → verify. Cheap, no Docker, and it is the first time CI would look at the tree the image actually ships.
Image build + tests/runtime-smoke/frameworks/sandbox/v1/smoke.sh remain workflow_dispatch against published runtimes. Dispatch base sandbox/v1 then runtime sandbox/v1 smoke before Brain; this PR does not change runtime-images/frameworks/sandbox/v1 (FROM sandbox-v1:v0.0.1-alpha.1-…).
|
@norberia thanks for the review — please take another look at b11ff58: the code fixes are pushed, including kill/retry recovery and real-source validation; per the requested behavior, the installer packages every Skill in the pinned norberia repository rather than requiring fixed names or counts. Checks are green, but base/runtime image builds and actual Devbox smoke are still pending before the paired Brain rollout, so this is a request for code re-review, not release approval. |
norberia
left a comment
norberia
left a comment
There was a problem hiding this comment.
Request changes (re-review of b11ff58)
The previous round is largely addressed. I re-ran the 20 Node tests and verify-pinned-source.mjs (3 Skills, 13 files, digest 1ad10ccb…). SIGKILL at both rename boundaries, dirty rebuilds, absolute wrapper binaries, ready-JSON validation, realpath CLI guard, GHCR node.js-22, and “no workspace mkdir unless --init-workspace” all check out. The no-allowlist decision is accepted as stated.
Do not pair this with Brain until the items below land and sandbox base then runtime is actually built and smoked. That last part is still a release gate, not claimed coverage.
Still must fix
-
recoverTransactionsturns post-publish leftovertxn-*into a hard fail on the next run.cleanupswallowsrmerrors, thenexists(transaction)throwsrecovery_cleanup_failedeven when liveskillsalready exists. Same-process mockedrm: first prepareready, secondrecovery_cleanup_failed. The regression test mocksrmonly in a child, then retries in the parent with a realrm. Iftargetexists, leftover txns are trash — best-effortrmonly. -
buildBundlefinallycan delete the new tree after wiping the old dest.rm(destination)thenrename(staging → dest)thenfinally rm(staging). If rename fails, both copies are gone. DockerRUNfailure is not committed; local/CI rebuild into the same dest is not safe. Don’trm(staging)after a successful promote; prefer rename-to-old + rename-to-dest + best-effort delete of old.
Should fix in the same pass
- Restore
backupwithout walking an incompletestage. A symlink/special file understagecurrently throwsbundle_symlinkand never restores custom Skills. - Journal sits in the git clone.
git clean -fdbetween kill and retry returnsreadywith only bundled Skills. Keep same-FS; move it out of the worktree (or under.agents/) and documentgit cleanas journal loss. - If live
skillsexists, leftoverskills-backup-N/skills-stage-Nshould be stale, notrecovery_ambiguousforever.
Already good on this commit
Kill/retry restore, dirty-build staging+verify, CI against the real Dockerfile pin, no workspace hashing, /usr/bin/{timeout,flock,node} + 28s shared budget + validate-ready, Chat --init-workspace vs deploy requires an existing workspace, GHCR Node builder.
Rollout (unchanged)
Publish sandbox-v1 base, then runtime (FROM sandbox-v1:v0.0.1-alpha.1-…, unchanged in this PR). Point Brain at the digest and drop nonempty DEPLOY_SKILL_SOURCE in the same deploy. Roll the pair back together.
| async function recoverTransactions(state, target) { | ||
| const transactions = (await readdir(state)).filter(name => name.startsWith('txn-')); | ||
| if (transactions.length > 1) fail('recovery_ambiguous'); | ||
| for (const name of transactions) { | ||
| const transaction = path.join(state, name); | ||
| await files(transaction, '', false); | ||
| const backup = path.join(transaction, 'backup'); | ||
| if (!(await exists(target))) { | ||
| if (!(await exists(backup))) fail('recovery_ambiguous'); | ||
| await rename(backup, target); | ||
| } | ||
| await cleanup(transaction); | ||
| if (await exists(transaction)) fail('recovery_cleanup_failed'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This reopens the previous “cleanup after publication must be best-effort” item.
cleanup swallows rm errors, then exists(transaction) still fails closed — including when live skills already exists (post-publish leftover). Same-process mocked rm on txn-*: first prepareBundle returns ready, the next call throws recovery_cleanup_failed.
The regression test only mocks rm in a child, then retries in the parent with a real rm, so it never sees this.
If target exists, leftover txn-* is trash. Best-effort rm only; do not throw. Fail closed only when skills is missing and backup restore cannot finish. Keep the mock across both prepares so the test actually covers retry.
| const staging = await mkdtemp(path.join(path.dirname(destination), '.skill-build-')); | ||
| try { | ||
| await mkdir(path.join(staging, 'skills')); | ||
| for (const name of names) await cp(path.join(sourceSkills, name), path.join(staging, 'skills', name), { recursive: true }); | ||
| const manifest = { schema: 1, revision, skills: names, files: await files(path.join(staging, 'skills')) }; | ||
| await writeFile(path.join(staging, 'manifest.json'), JSON.stringify(manifest)); | ||
| await verifyBundle(staging); | ||
| await rm(destination, { recursive: true, force: true }); | ||
| await rename(staging, destination); | ||
| return (await verifyBundle(destination)).digest; | ||
| } finally { | ||
| await rm(staging, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
On success staging was renamed away, so finally is a no-op. If rename fails after rm(destination), finally still deletes staging. Probe against an existing dest: previous bundle and new staging both gone (destExists: false).
Docker RUN failure is not committed, so the image path is mostly safe. Local/CI rebuild into the same dest is not.
Do not rm(staging) after a successful promote. Better: rename(dest → dest.old), rename(staging → dest), best-effort rm(dest.old) so a failed promote can still recover the previous bundle.
| const transaction = path.join(state, name); | ||
| await files(transaction, '', false); | ||
| const backup = path.join(transaction, 'backup'); | ||
| if (!(await exists(target))) { | ||
| if (!(await exists(backup))) fail('recovery_ambiguous'); | ||
| await rename(backup, target); | ||
| } |
There was a problem hiding this comment.
files(transaction) walks the whole txn, including an incomplete stage, before restoring backup. README says stages may be incomplete and must not be promoted. A symlink/special file under stage throws bundle_symlink and never rename(backup → skills).
Probe: target gone, backup has custom/keep, stage has a symlink → bundle_symlink, custom not restored.
Type-scan and restore backup only. Delete stage best-effort. Do not require the stage tree to be a valid Skill dir.
| const agentRoot = path.join(workspace, '.agents'); | ||
| await safeDirectory(agentRoot, true); | ||
| const target = path.join(agentRoot, 'skills'); | ||
| const state = path.join(workspace, '.sealai-skill-transactions'); |
There was a problem hiding this comment.
Journal is inside the git clone (/home/devbox/project/.sealai-skill-transactions) so same-FS rename works. After SIGKILL-at-backup, git status shows backup/stage as untracked; git clean -fd removes both .agents/ and the journal. The next prepare returns ready with only bundled Skills — same silent custom-Skill loss as the original bug, if the worktree is cleaned between kill and retry.
Keep same-FS, move the journal out of the clone (e.g. /home/devbox/.sealai-skill-transactions if that device matches project). If it must stay in the project, put it under .agents/ and treat git clean as journal loss in the README.
Related: recoverLegacy fail-closes when live skills exists alongside leftover skills-backup-N / skills-stage-N. If target exists, those names are stale; ignore or delete them. Restore only when skills is missing.
2 participants
Summary
Companion Brain PR: labring/brain#340
https://github.com/norberia/sealos-skills-next.gitat commitbdd824cf2fd6c72896f8e201f32259cc8aed3f98(canonicalplugins/sealos/skillsdirectory) into the sandbox/v1 image with a versioned SHA-256 manifest.Validation
use-sealos,sealos-deploy,k8s-kaniko-job).Rollout / rollback
Publish and validate the bundled sandbox image before rolling out the companion Brain change. Brain must select its immutable image reference and remove nonempty DEPLOY_SKILL_SOURCE overrides. Drain active/blocked deployment tasks before switching; roll back the Brain/runtime image pair together.
Recovery journals outside
.agentsrestore backups before preparing after a killed process; ambiguous state fails closed. Post-publication cleanup is best-effort. This is an operational integrity check, not an adversarial filesystem boundary or power-loss durability guarantee.Review follow-up
Per maintainer direction, install every Skill in the pinned repository. No fixed Skill names or counts are required. The source directory determines the manifest, and smoke compares installed files/count against that manifest. CI now fetches the actual Dockerfile pin and performs build, verify, repeated preparation, and byte-for-byte source comparison (passed locally: 3 Skills, 13 files).
The wrapper uses absolute system binaries, clears Node environment overrides, validates readiness JSON, and shares a 28-second budget across lock waiting and preparation (one-second kill grace). Deployment preparation requires an existing workspace; Chat explicitly opts into initialization with
--init-workspacein the companion PR. The build stage reuses the GHCR Node base and verifies the final bundle.Base-image build, derived runtime-image build, and actual Devbox smoke remain pending release gates. No image was published or deployment changed in this update.