Skip to content

fix(skills)!: prepare bundled runtime skills before agent startup - #340

Merged
zjy365 merged 3 commits into
mainfrom
codex/bundled-runtime-skills
Sep 8, 2026
Merged

fix(skills)!: prepare bundled runtime skills before agent startup#340
zjy365 merged 3 commits into
mainfrom
codex/bundled-runtime-skills

Conversation

@zjy365

@zjy365 zjy365 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Runtime dependency: labring-actions/devbox-runtime#154

  • Replace managed Chat/Deployment Task network Skill installation with the runtime-owned offline preparation helper (30-second exec cap).
  • Await Skill discovery before assembling the first Chat toolset; retain progressive loadSkill/loadSkillResource loading and return the full Skill directory.
  • Include the runtime image reference in Chat runtime identity, reject legacy source overrides, and update configuration, failure wording and ADR 0037.

Validation

  • Rebased on current main (including the AI SDK v7 upgrade).
  • 83 focused Skills/Chat/Deployment tests passed.
  • 66 Chat route tests passed.
  • bun typecheck and bun check passed.
  • Docker image build and actual Devbox end-to-end verification remain pending because the local Docker daemon is unavailable.

Breaking configuration / release gate

Do not deploy this Brain change against an old runtime image. First publish and validate a sandbox/v1 image containing the offline bundle, set DEVBOX_RUNTIME_IMAGE to its immutable reference, and remove nonempty DEPLOY_SKILL_SOURCE overrides. Missing bundles fail closed; there is no network fallback.

Finish active/blocked Deployment Tasks before switching versions. Old Chat runtimes retain their existing lifecycle and data. Roll back the Brain/runtime image pair together. No database migration or codex-gateway change.

Review follow-up

Chat now explicitly passes --init-workspace because it has no repository clone. Deployment Tasks omit it and require their workspace to exist. This matches the Runtime PR's stricter preparation contract without breaking Chat cold starts. Source Skill names and counts are determined by the pinned repository, not an installer allowlist.

Replace managed runtime network installation with the image-owned offline helper and await Chat discovery.

BREAKING CHANGE: managed runtimes require a bundled-skills sandbox image; remove DEPLOY_SKILL_SOURCE overrides.
@norberia

norberia commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@cursoragent review this

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Request changes on PR #340. Offline helper + fail-closed is the right contract; two issues block merge. Structured walkthrough: PR #340 评审. I also posted this on the PR.

Must fix

  • Every Chat turn re-prepares Skills. buildChatToolset always await warmChatDevboxSkills(), which never reads the snapshot it publishes and drops the in-flight promise on settle. Page-load warmup is not an optimization. The same request also uses two sandboxes, so the first loadSkill/bash pays installChatSkills again.
  • CI lint is red on this commit. turbo/no-undeclared-env-vars: tests touch DEVBOX_RUNTIME_IMAGE (never in turbo.json) and DEPLOY_SKILL_SOURCE (removed from turbo.json here).

Keep: no npx fallback, image in Chat identity, sanitized prepare errors, loadSkill returning the real skillDirectory.

Release note: leftover nonempty DEPLOY_SKILL_SOURCE fails Chat and blocked Deploy resume; empty DEVBOX_RUNTIME_IMAGE still falls through to the provider default until test -x fails. Unset the override and pin an immutable bundled image before upgrading Brain.

Open in Web Open in Cursor 

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Request changes

The managed-runtime switch is the right contract: offline /usr/local/bin/sealai-prepare-skills, reject nonempty DEPLOY_SKILL_SOURCE, put the image into Chat runtime identity, fail closed on empty discovery. Two issues block merge.

1. Background warmup is not an optimization

ADR 0037 and the comment on buildChatToolset say page-load warmup is only a latency optimization, and Chat should await successful discovery before the first toolset.

warmChatDevboxSkills never reads chatDevboxSkillSnapshots. On settle it deletes the in-flight promise (chat-runtime.ts ~88–90). buildChatToolset always await warmChatDevboxSkills() (tools.ts ~82). Every Chat turn therefore repeats ensureChatDevbox + sealai-prepare-skills + discovery before OpenAI connect / first SSE token.

The same request also uses two sandboxes: warmup’s sandbox is stop()’d, then loadSkill / bash use a different createChatDevboxSandbox and pay installChatSkills again (no install marker anymore).

Fix: hit the snapshot on the request path (await in-flight warmup only on miss). Share one sandbox, or skip prepare when this process already prepared that runtime. Add a test: after warmup completes, a second buildChatToolset must not increase Devbox exec count.

2. CI lint is red on this commit

@sealai/ui#lint / turbo/no-undeclared-env-vars:

  • chat-runtime.test.ts reads DEVBOX_RUNTIME_IMAGE (never in turbo.json)
  • runner.github-ai-proxy.test.ts reads DEPLOY_SKILL_SOURCE (removed from turbo.json in this PR)

Declare DEVBOX_RUNTIME_IMAGE in globalEnv. Either keep DEPLOY_SKILL_SOURCE declared for the reject tests, or inject env into assertBundledSkillsConfiguration without touching process.env.

Release gate (not a logic bug, but the code does not match the docs)

  • .env.example calls DEVBOX_RUNTIME_IMAGE required; Helm default is ""; Brain omits createDevbox.image and uses the provider default. Fail-closed happens at test -x, not at boot.
  • Leftover nonempty DEPLOY_SKILL_SOURCE in private Helm values fails Chat and blocked Deploy resume as deploy-configuration-invalid (ADR 0042 hides the “no longer supported” text). Drain/unset that override before upgrading Brain.

What looks correct and should stay: no npx fallback, existing Chat Devboxes still run the helper, image change creates a new Chat identity, prepare stderr is sanitized, loadSkill now returns the real skillDirectory.

I did not run Docker/Devbox e2e (as the PR notes, no local daemon).

@norberia

norberia commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@cursoragent review this

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Still request changes on PR #340 at c5b1fdcb. The follow-up only adds --init-workspace; both previous merge blockers are untouched. Walkthrough: PR #340 复审.

Still blocking

  • Every Chat turn still re-runs ensure + prepare + discover; snapshot is never read; two sandboxes remain.
  • Same 7 turbo/no-undeclared-env-vars lint errors; turbo.json was not updated.

Follow-up is correct: Chat passes --init-workspace (no clone); Deploy omits it (workspace must already exist). Runtime helper mkdir-if-missing — it does not wipe the project. Keep that split. ADR should cite the image pin, not the personal norberia/sealos-skills-next fork.

Open in Web Open in Cursor 

@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Re-review c5b1fdcb — still request changes

The follow-up correctly splits Chat vs Deploy helper invocation. It does not address the two merge blockers from the previous review.

Still blocking

  1. Every Chat turn still re-prepares. warmChatDevboxSkills still never reads chatDevboxSkillSnapshots and still deletes the in-flight promise on settle (chat-runtime.ts). buildChatToolset still always awaits it (tools.ts). Two sandboxes remain, so the first loadSkill/bash still pays installChatSkills again. This commit only adds --init-workspace onto those repeated prepares.

  2. CI lint is still red on this head (turbo/no-undeclared-env-vars, same 7 errors). turbo.json was not touched: declare DEVBOX_RUNTIME_IMAGE; keep DEPLOY_SKILL_SOURCE declared for the reject tests or stop reading process.env.DEPLOY_SKILL_SOURCE.

Follow-up is fine

--init-workspace on Chat / omitted on Deploy matches runtime PR 154: missing workspace mkdir for Chat; Deploy fail-closed if the directory does not exist after clone. The flag does not wipe /home/devbox/project (mkdir-if-missing, then the same prepareBundle). Keep this split.

Nits, not blockers: named option instead of buildSealosSkillsInstallCommand(true); assert the flag on the Chat exec command and its absence on Deploy; ADR should name the image-owned pin, not the personal fork norberia/sealos-skills-next as product source of truth.

@zjy365
zjy365 requested a review from norberia September 8, 2026 02:30
@zjy365

zjy365 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

@norberia thanks for the review — please take another look at 384c2ee: Chat now reuses discovery snapshots and skips preparation for a previously prepared Devbox generation, with regression coverage for repeated turns, first Skill loading, and runtime replacement; the missing Turbo environment declarations are also restored. All 54 focused tests, bun typecheck, bun check, and bun lint pass locally; the new CI run and runtime image/Devbox smoke remain separate validation steps.

@cursoragent review this

@norberia

norberia commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@cursoragent review this

@cursor

cursor Bot commented Sep 8, 2026

Copy link
Copy Markdown

Approve 384c2eec — both previous merge blockers are fixed. Structured walkthrough: PR #340 复审. I also posted this on the PR.

Fixed

  • Chat now hits discovery snapshots; a second buildChatToolset does not increase Devbox exec count. First loadSkill skips sealai-prepare-skills when the Devbox generation matches.
  • turbo.json declares DEVBOX_RUNTIME_IMAGE and DEPLOY_SKILL_SOURCE; Quality lint is green.

Keep: Chat --init-workspace / Deploy omit it; no npx fallback; image in Chat identity; fail-closed on empty discovery.

Nits, not blockers: ADR 0037 still names the personal norberia/sealos-skills-next fork. Helm empty DEVBOX_RUNTIME_IMAGE still boots and fails later at test -x — pin an immutable sandbox/v1 image and unset leftover DEPLOY_SKILL_SOURCE before upgrading Brain.

Open in Web Open in Cursor 

@cursor

cursor Bot commented Sep 8, 2026

Copy link
Copy Markdown

Approve PR #340 at 384c2eec. Both previous merge blockers are fixed. Walkthrough: PR #340 复审.

Fixed

  • Chat request path now reads discovery snapshots; a second buildChatToolset does not increase Devbox exec count. First loadSkill skips sealai-prepare-skills when the Devbox generation matches.
  • turbo.json declares DEVBOX_RUNTIME_IMAGE and DEPLOY_SKILL_SOURCE; Quality lint is green.

Keep: Chat --init-workspace / Deploy omit it; no npx fallback; image in Chat identity; fail-closed on empty discovery.

Release: pin an immutable bundled sandbox/v1 image and unset nonempty DEPLOY_SKILL_SOURCE before upgrading Brain. Helm empty DEVBOX_RUNTIME_IMAGE still boots and fails later at test -x.

@cursor

cursor Bot commented Sep 8, 2026

Copy link
Copy Markdown

Approve 384c2eec

The two merge blockers from the previous reviews are actually fixed. CI Quality (check / typecheck / lint) is green on this head.

1. Chat no longer re-prepares every turn — fixed

warmChatDevboxSkills now reads chatDevboxSkillSnapshots first and only awaits an in-flight warmup on miss. Settle still deletes the warmup promise; the snapshot stays.

buildChatToolset still await warmChatDevboxSkills(), which is now a memory hit after the first success. The requested regression is present: after warmup, a second buildChatToolset does not increase Devbox exec count.

The warmup sandbox is still stop()'d. loadSkill / bash still open a second sandbox and still call installChatSkills. The paid sealai-prepare-skills exec is skipped when this process already prepared that name|creationTimestamp. Replacement or a missing timestamp invalidates the skip and the snapshot.

Covered in background Skill warmup is shared and publishes metadata after discovery:

  • concurrent misses share one promise
  • snapshot hit → exec count unchanged
  • second buildChatToolset → exec count unchanged
  • first loadSkillprepareCalls unchanged
  • new creationTimestamp → prepare again
  • creationTimestamp = null never skips

2. turbo/no-undeclared-env-vars — fixed

turbo.json globalEnv now includes DEVBOX_RUNTIME_IMAGE and DEPLOY_SKILL_SOURCE. Tests still assign process.env; lint is green.

Keep

Named { initializeWorkspace: true } for Chat; Deploy buildSealosSkillsInstallCommand() omits it. No npx fallback. Image is in Chat runtime identity. Nonempty DEPLOY_SKILL_SOURCE is rejected. Prepare stderr is sanitized. loadSkill returns the real skillDirectory. Empty discovery does not publish a snapshot. Failed prepare does not stick.

Nits (do not re-block)

  • ADR 0037:37 still says the helper installs from the pinned norberia/sealos-skills-next source. The pin is the image digest / DEVBOX_RUNTIME_IMAGE, not a personal fork.
  • Helm default DEVBOX_RUNTIME_IMAGE: "" still omits createDevbox.image and uses the provider default. Fail-closed is test -x, not boot. Same release gate as before: publish and pin an immutable sandbox/v1 image, unset leftover DEPLOY_SKILL_SOURCE, drain active/blocked Deployment Tasks, roll Brain + runtime together.

I did not run Docker/Devbox e2e (as the PR notes, no local daemon). Skip-prepare is process-local; each replica prepares once.

@zjy365
zjy365 merged commit 8f9c0ce into main Sep 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants