From 6f379a884f172aa0d10ff3d18c4c9401a2d52893 Mon Sep 17 00:00:00 2001 From: core-devops Date: Tue, 16 Jun 2026 14:08:01 -0700 Subject: [PATCH 1/2] RFC#2843 #32: agent-skills are plugins, not asset-channel paths; record declared plugins on create_workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two evidence-backed prod fixes that together make a fresh seo-agent auto-install seo-all post-boot. FIX A — agent-skills must NOT ride the provisioning asset channel. IsCPTemplateAssetPath still allowed `agent-skills/*`, so a fresh seo-agent's ~501-716 KiB skill tree got pulled into the provision request and fail-closed before the CP was ever called (WORKSPACE_PROVISION_FAILED, reproduced twice; zero CP provision requests logged for the failed ws). Skills are PLUGINS now (core #2995): they install dynamically post-online via the gitea:// plugin resolver, which reads the agent-skills/ subpath from the template repo at install time. So the asset-channel allowlist is now config.yaml + prompts/** ONLY; agent-skills/* is rejected at the addAsset boundary. The skill files remain in the template repo (they ARE the plugin source). The seo-agent asset set drops to config.yaml (~9 KB) + prompts/seo-agent.md (~8 KB) = ~18 KB, far under the 256 KiB cap. FIX B — create_workspace must record declared plugins. The post-online reconcile only installs plugins that have workspace_declared_plugins rows. recordDeclaredPlugin ran ONLY in org_import.go, not in the single create_workspace path — so a singly provisioned seo-agent got no declared rows, the reconcile no-op'd, and seo-all never installed. New template_plugins.go parses the template config.yaml `plugins:` block (mergePlugins-aligned dedup + "!"/"-" opt-out) and records each declared plugin in WorkspaceHandler.Create, mirroring the org_import.go loop. Idempotent via the ON CONFLICT upsert. Tests: - provisioner: agent-skills/* now REJECTED by IsCPTemplateAssetPath + collectCPConfigFiles + the gitea fetcher (flipped the prior "allows agent-skills" tests to rejection/skip tests; updated the wire-shape + large-asset tests to use config.yaml/prompts only). - handlers: new template_plugins_test.go proves parseTemplatePlugins reads the seo-agent shape + dedup/opt-out, and (sqlmock-backed) seedTemplatePlugins WRITES the workspace_declared_plugins row with the derived install name (seo-all) — recordDeclaredPlugin no-ops on nil db.DB, so a DB-backed test is required to prove the write. - e2e: test_template_delivery_e2e.sh already asserts the new two-channel contract (provision succeeds, config.yaml non-stub + prompts via the asset channel, agent-skills NOT on the old asset path [negative control], seo-all installs via the post-online plugin reconcile). Added the FIX-B source paths (workspace.go, template_plugins.go) to the workflow path filter so the gate fires when this code changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/workflows/template-delivery-e2e.yml | 6 +- .../internal/handlers/template_plugins.go | 136 +++++++++++++++++ .../handlers/template_plugins_test.go | 143 ++++++++++++++++++ .../internal/handlers/workspace.go | 24 ++- .../internal/handlers/workspace_provision.go | 6 +- .../internal/provisioner/cp_provisioner.go | 90 ++++++----- .../provisioner/cp_provisioner_test.go | 35 +++-- .../provisioner/gitea_template_assets.go | 28 ++-- .../provisioner/gitea_template_assets_test.go | 58 ++++--- .../internal/provisioner/provisioner.go | 36 ++--- .../internal/provisioner/template_assets.go | 55 ++++--- .../provisioner/template_assets_test.go | 106 ++++++++----- 12 files changed, 555 insertions(+), 168 deletions(-) create mode 100644 workspace-server/internal/handlers/template_plugins.go create mode 100644 workspace-server/internal/handlers/template_plugins_test.go diff --git a/.gitea/workflows/template-delivery-e2e.yml b/.gitea/workflows/template-delivery-e2e.yml index 02ff435b..27dc2567 100644 --- a/.gitea/workflows/template-delivery-e2e.yml +++ b/.gitea/workflows/template-delivery-e2e.yml @@ -36,6 +36,8 @@ on: - 'workspace-server/internal/handlers/platform_agent.go' - 'workspace-server/cmd/server/main.go' - 'workspace-server/internal/handlers/org_import.go' + - 'workspace-server/internal/handlers/workspace.go' + - 'workspace-server/internal/handlers/template_plugins.go' - 'workspace-server/internal/handlers/plugins_reconcile.go' - 'workspace-server/internal/handlers/plugins_install_pipeline.go' - 'workspace-server/internal/handlers/plugins_tracking.go' @@ -51,6 +53,8 @@ on: - 'workspace-server/internal/handlers/platform_agent.go' - 'workspace-server/cmd/server/main.go' - 'workspace-server/internal/handlers/org_import.go' + - 'workspace-server/internal/handlers/workspace.go' + - 'workspace-server/internal/handlers/template_plugins.go' - 'workspace-server/internal/handlers/plugins_reconcile.go' - 'workspace-server/internal/handlers/plugins_install_pipeline.go' - 'workspace-server/internal/handlers/plugins_tracking.go' @@ -65,7 +69,7 @@ concurrency: jobs: delivery: - name: Template-asset delivery (fresh seo-agent boots WITH skills) + name: Template-asset delivery (fresh seo-agent: config+prompts via asset channel, seo-all via plugin reconcile) runs-on: ubuntu-latest # Phase 1: advisory. Remove this line in Phase 2 to make it merge-blocking. # mc#2996 — Phase 2 promotion tracker (remove continue-on-error; forced 14d renewal cadence). diff --git a/workspace-server/internal/handlers/template_plugins.go b/workspace-server/internal/handlers/template_plugins.go new file mode 100644 index 00000000..b2fdf8e6 --- /dev/null +++ b/workspace-server/internal/handlers/template_plugins.go @@ -0,0 +1,136 @@ +package handlers + +// template_plugins.go — read a workspace template's `plugins:` block and +// record the DECLARED plugin set (workspace_declared_plugins) for a workspace +// created directly via WorkspaceHandler.Create. Mirrors the org/import flow +// (org_import.go), so a singly-provisioned workspace lands with the same +// declared rows the org/import path would have produced. +// +// Why this exists (RFC#2843 #32): the post-online reconcile +// (ReconcileWorkspacePlugins) only installs plugins that have +// workspace_declared_plugins rows. recordDeclaredPlugin previously ran ONLY in +// org_import.go, NOT in the single create_workspace path. So a seo-agent +// created via Create (template="seo-agent") got NO declared rows → the +// reconcile no-op'd → the seo-all skill (now a plugin, per #32) never +// installed. This module closes that gap by parsing the template config.yaml's +// `plugins:` block at create time and recording each declared plugin. +// +// CONTRACT (matches org_import.go): +// - Each `plugins:` entry is a source-contract string (e.g. +// "gitea://owner/repo/subpath#ref" or a bare local name). The install +// name is derived from the source via plugins.PluginNameFromSource so the +// reconcile can diff declared-vs-installed without fetching. +// - Entries are de-duplicated by raw source (mergePlugins semantics: a +// single template config.yaml's `plugins:` list is already the "merged" +// set, so there are no defaults/per-ws halves to union here — but we run +// it through mergePlugins(nil, list) to apply the same dedup + "!"/"-" +// opt-out handling the org path uses, keeping the two paths byte-aligned). +// - recordDeclaredPlugin upserts ON CONFLICT (idempotent across re-creates), +// so this is safe to call on every Create. +// +// Hostile-template defenses are shared with template_schedules.go: the +// config.yaml is read through the same maxTemplateConfigYAMLBytes LimitReader +// so a YAML anchor-bomb cannot pre-explode memory before unmarshal returns. + +import ( + "context" + "errors" + "fmt" + "io" + "log" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" + + "git.moleculesai.app/molecule-ai/molecule-core/workspace-server/internal/plugins" +) + +// maxTemplatePlugins bounds how many plugin entries a single template may +// declare. Generous relative to legitimate use (production templates declare +// 1–3 plugins); the cap exists only so a hostile/buggy template can't enqueue +// an unbounded declared set. +const maxTemplatePlugins = 100 + +// templateConfigPlugins is the minimal shape parsed from a workspace template's +// config.yaml. Only the top-level `plugins:` block is modelled; the rest of the +// file is opaque to this loader (parsed elsewhere for schedules, runtime_config, +// etc.). +type templateConfigPlugins struct { + Plugins []string `yaml:"plugins"` +} + +// parseTemplatePlugins reads `/config.yaml` and returns its +// `plugins:` block (nil + nil error when the file is absent or the block is +// empty). The file is read through the shared maxTemplateConfigYAMLBytes +// LimitReader. Returns an error only when a present config.yaml fails to read +// or parse — callers should treat that as a template-author bug and continue +// (a broken plugins block must never block workspace provisioning). +func parseTemplatePlugins(templatePath string) ([]string, error) { + if templatePath == "" { + return nil, nil + } + f, err := os.Open(filepath.Join(templatePath, "config.yaml")) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + return nil, fmt.Errorf("open template config.yaml: %w", err) + } + defer f.Close() + + data, err := io.ReadAll(io.LimitReader(f, maxTemplateConfigYAMLBytes+1)) + if err != nil { + return nil, fmt.Errorf("read template config.yaml: %w", err) + } + if int64(len(data)) > maxTemplateConfigYAMLBytes { + return nil, fmt.Errorf("template config.yaml exceeds %d-byte cap", maxTemplateConfigYAMLBytes) + } + var cfg templateConfigPlugins + if err := yaml.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("parse template config.yaml plugins: %w", err) + } + // A single template's `plugins:` list IS the merged set — there are no + // defaults/per-ws halves to union (that's the org-import-only case). Run it + // through mergePlugins(nil, list) anyway so dedup + "!"/"-" opt-out handling + // stays byte-identical to the org path. + merged := mergePlugins(nil, cfg.Plugins) + if len(merged) > maxTemplatePlugins { + return nil, fmt.Errorf("template declares %d plugins; cap is %d", len(merged), maxTemplatePlugins) + } + return merged, nil +} + +// seedTemplatePlugins records each declared plugin source (workspace_declared_plugins) +// for a Create-provisioned workspace. Returns (recorded, skipped) counts so the +// caller can observe partial-record states. Mirrors the org_import.go loop: +// - derive the install name via plugins.PluginNameFromSource, +// - warn on a name collision (two sources collapsing to the same install +// name — the latter wins via the ON CONFLICT upsert), +// - recordDeclaredPlugin upserts the (workspace_id, plugin_name, source) row. +// +// Per-entry failures are logged and skipped so one bad entry never blocks the +// rest of the declared set. +func seedTemplatePlugins(ctx context.Context, workspaceID string, sources []string) (recorded, skipped int) { + seenPluginNames := map[string]string{} // name → first source that claimed it + for _, pluginSource := range sources { + pluginName, nameErr := plugins.PluginNameFromSource(pluginSource) + if nameErr != nil { + log.Printf("Create %s: skipping plugin %q — cannot derive install name: %v", workspaceID, pluginSource, nameErr) + skipped++ + continue + } + if prevSource, dup := seenPluginNames[pluginName]; dup && prevSource != pluginSource { + log.Printf("Create %s: WARNING plugin name collision — %q and %q both derive name %q; the latter overwrites the former", + workspaceID, prevSource, pluginSource, pluginName) + } + seenPluginNames[pluginName] = pluginSource + if recErr := recordDeclaredPlugin(ctx, workspaceID, pluginName, pluginSource); recErr != nil { + log.Printf("Create %s: failed to record declared plugin %s (%s): %v", workspaceID, pluginName, pluginSource, recErr) + skipped++ + continue + } + recorded++ + } + return recorded, skipped +} diff --git a/workspace-server/internal/handlers/template_plugins_test.go b/workspace-server/internal/handlers/template_plugins_test.go new file mode 100644 index 00000000..1da81102 --- /dev/null +++ b/workspace-server/internal/handlers/template_plugins_test.go @@ -0,0 +1,143 @@ +package handlers + +// template_plugins_test.go — unit tests for parseTemplatePlugins + +// seedTemplatePlugins (RFC#2843 #32). Proves a workspace created directly via +// WorkspaceHandler.Create from a template that declares plugins: +// 1. parses the template config.yaml's `plugins:` block, and +// 2. WRITES the workspace_declared_plugins rows the post-online reconcile +// needs (the gap this change closes: recordDeclaredPlugin previously ran +// only in the org/import path, so a singly-provisioned seo-agent got no +// declared rows → reconcile no-op → seo-all never installed). + +import ( + "context" + "path/filepath" + "testing" + + "github.com/DATA-DOG/go-sqlmock" +) + +func TestParseTemplatePlugins_AbsentFile(t *testing.T) { + got, err := parseTemplatePlugins(t.TempDir()) + if err != nil { + t.Fatalf("expected nil error for absent config.yaml, got %v", err) + } + if got != nil { + t.Fatalf("expected nil slice, got %#v", got) + } +} + +func TestParseTemplatePlugins_EmptyPath(t *testing.T) { + got, err := parseTemplatePlugins("") + if err != nil { + t.Fatalf("expected nil error for empty path, got %v", err) + } + if got != nil { + t.Fatalf("expected nil slice for empty path, got %#v", got) + } +} + +func TestParseTemplatePlugins_NoPluginsBlock(t *testing.T) { + dir := t.TempDir() + mustWriteFile(t, filepath.Join(dir, "config.yaml"), ` +name: Some Template +runtime: claude-code +model: foo/bar +`) + got, err := parseTemplatePlugins(dir) + if err != nil { + t.Fatalf("expected nil error when plugins: absent, got %v", err) + } + if len(got) != 0 { + t.Fatalf("expected zero plugins, got %d: %v", len(got), got) + } +} + +// TestParseTemplatePlugins_SeoAgentShape pins the real seo-agent template +// config.yaml shape: a top-level `plugins:` list with the seo-all gitea source. +func TestParseTemplatePlugins_SeoAgentShape(t *testing.T) { + dir := t.TempDir() + mustWriteFile(t, filepath.Join(dir, "config.yaml"), ` +name: SEO Agent +runtime: claude-code +plugins: + - gitea://molecule-ai/molecule-ai-workspace-template-seo-agent/agent-skills/seo-all#main +runtime_config: + model: moonshot/kimi-k2.6 +`) + got, err := parseTemplatePlugins(dir) + if err != nil { + t.Fatalf("parseTemplatePlugins: %v", err) + } + want := "gitea://molecule-ai/molecule-ai-workspace-template-seo-agent/agent-skills/seo-all#main" + if len(got) != 1 || got[0] != want { + t.Fatalf("expected [%q], got %v", want, got) + } +} + +// TestParseTemplatePlugins_DedupAndOptOut pins the mergePlugins-aligned +// semantics: duplicate sources collapse and a leading "!"/"-" opts a plugin +// out (matching the org/import path). +func TestParseTemplatePlugins_DedupAndOptOut(t *testing.T) { + dir := t.TempDir() + mustWriteFile(t, filepath.Join(dir, "config.yaml"), ` +name: T +plugins: + - local://alpha + - local://beta + - local://alpha + - "!local://beta" +`) + got, err := parseTemplatePlugins(dir) + if err != nil { + t.Fatalf("parseTemplatePlugins: %v", err) + } + if len(got) != 1 || got[0] != "local://alpha" { + t.Fatalf("expected dedup + beta opted out → [local://alpha], got %v", got) + } +} + +// TestSeedTemplatePlugins_WritesDeclaredRows is the load-bearing FIX-B proof: +// seedTemplatePlugins derives the install name from each source and UPSERTS a +// workspace_declared_plugins row. Uses sqlmock so the actual INSERT (and its +// derived plugin_name) is asserted — recordDeclaredPlugin no-ops when db.DB is +// nil, so a DB-backed test is required to prove the write really happens. +func TestSeedTemplatePlugins_WritesDeclaredRows(t *testing.T) { + mock, cleanup := withMockDB(t) + defer cleanup() + + source := "gitea://molecule-ai/molecule-ai-workspace-template-seo-agent/agent-skills/seo-all#main" + // gitea source with subpath → install name is the last subpath segment: + // "seo-all". The upsert keys on (workspace_id, plugin_name) and stores the + // full source string in source_raw. + mock.ExpectExec(`INSERT INTO workspace_declared_plugins`). + WithArgs("ws-create-1", "seo-all", source). + WillReturnResult(sqlmock.NewResult(1, 1)) + + recorded, skipped := seedTemplatePlugins(context.Background(), "ws-create-1", []string{source}) + if recorded != 1 || skipped != 0 { + t.Fatalf("expected recorded=1 skipped=0, got recorded=%d skipped=%d", recorded, skipped) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet DB expectations (the declared row was not written as expected): %v", err) + } +} + +// TestSeedTemplatePlugins_SkipsUnparseableSource proves a bad source is skipped +// (logged) rather than aborting the rest of the declared set — and crucially +// does NOT issue a DB write for it. +func TestSeedTemplatePlugins_SkipsUnparseableSource(t *testing.T) { + mock, cleanup := withMockDB(t) + defer cleanup() + + // A scheme with no known naming rule → PluginNameFromSource errors → skip. + // No ExpectExec is programmed; sqlmock fails the test if any unexpected + // INSERT fires. + recorded, skipped := seedTemplatePlugins(context.Background(), "ws-create-1", []string{"bogus-scheme://whatever"}) + if recorded != 0 || skipped != 1 { + t.Fatalf("expected recorded=0 skipped=1 for an unparseable source, got recorded=%d skipped=%d", recorded, skipped) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet DB expectations: %v", err) + } +} diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index abb29bf7..8a5a1cd6 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -105,7 +105,7 @@ type WorkspaceHandler struct { // state used by maybeMarkContainerDead. A transient A2A forward error // or a single flaky IsRunning probe must not recycle a recently-alive // container (#2929). Protected because ProxyA2A is called concurrently. - deadProbeMu sync.Mutex + deadProbeMu sync.Mutex deadProbeAttempts map[string]deadProbeRecord // asyncWG tracks goroutines launched by goAsync so tests can wait @@ -1009,6 +1009,28 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { } } + // Record the template's declared plugins (RFC#2843 #32) so the post-online + // reconcile (ReconcileWorkspacePlugins) installs them once the box is + // reachable. agent-skills are PLUGINS now — they install dynamically + // post-online, NOT via the provisioning channel — so a Create that omits + // this step leaves workspace_declared_plugins empty, the reconcile no-ops, + // and (e.g.) seo-all never installs. This mirrors the org_import.go loop; + // recordDeclaredPlugin upserts ON CONFLICT so it is idempotent across + // re-creates. Non-fatal: a broken plugins block must never block + // provisioning (the workspace row is already live). + if provisionOK && templatePath != "" { + if declaredPlugins, parseErr := parseTemplatePlugins(templatePath); parseErr != nil { + log.Printf("Create %s: parsing template plugins: %v (continuing)", id, parseErr) + } else if len(declaredPlugins) > 0 { + recorded, skipped := seedTemplatePlugins(ctx, id, declaredPlugins) + if skipped > 0 { + log.Printf("Create %s: template declared-plugin partial-record: recorded=%d skipped=%d total=%d", id, recorded, skipped, len(declaredPlugins)) + } else { + log.Printf("Create %s: recorded %d/%d template declared plugins", id, recorded, len(declaredPlugins)) + } + } + } + // Mint the workspace's first bearer token and return it inline // (#1644). Pre-fix, callers had to make a separate POST to // /admin/workspaces/:id/tokens (production path, AdminAuth-gated, diff --git a/workspace-server/internal/handlers/workspace_provision.go b/workspace-server/internal/handlers/workspace_provision.go index 83029c8b..1ce57abe 100644 --- a/workspace-server/internal/handlers/workspace_provision.go +++ b/workspace-server/internal/handlers/workspace_provision.go @@ -415,8 +415,10 @@ func templateIdentityForRuntimeOrEmpty(runtime string) string { // template VARIANT like seo-agent has runtime="claude-code" but // template="seo-agent"; keying the fetch on runtime looked up // templateRepoByName["claude-code"] (no such key) → empty identity → the -// fetcher delivered NOTHING, so agent-skills/seo-all never reached the box -// (config.yaml + prompts arrived via the legacy SM path, masking it). #32. +// fetcher delivered NOTHING, so the seo-agent box got a stub config.yaml. +// With this fix the seo-agent identity resolves and config.yaml + prompts +// arrive via the asset channel. (agent-skills/seo-all is no longer carried +// here at all — RFC#2843 #32: skills are plugins, installed post-online.) // Falls back to runtime for the common case where runtime==template name // (hermes/codex/openclaw/google-adk), and to "" when neither resolves (external // runtimes — collectCPConfigFiles treats empty identity as "skip the fetcher"). diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index 25cce29b..b9326107 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -177,10 +177,10 @@ type cpProvisionRequest struct { // differing ONLY in image + config overlay). Omitted when empty so the // wire shape is unchanged for ordinary workspaces; an older CP simply // ignores the field. - Kind string `json:"kind,omitempty"` - Display WorkspaceDisplayConfig `json:"display,omitempty"` - PlatformURL string `json:"platform_url"` - Env map[string]string `json:"env"` + Kind string `json:"kind,omitempty"` + Display WorkspaceDisplayConfig `json:"display,omitempty"` + PlatformURL string `json:"platform_url"` + Env map[string]string `json:"env"` // ConfigFiles are template + generated config files to write into the // EC2 instance's /configs directory. OFFSEC-010: collected by // collectCPConfigFiles which rejects symlinks and non-regular files @@ -194,15 +194,16 @@ type cpProvisionRequest struct { ConfigFiles map[string]string `json:"config_files,omitempty"` // TemplateAssets (RFC #2843 #24) are non-secret template assets - // (config.yaml + prompts/ + agent-skills/) fetched from a - // non-secret asset channel (template repo / Gitea shallow clone per - // RFC §4.2 transport option (a)). They travel on a SEPARATE wire - // field from ConfigFiles (the SM-bound bundle) so a future CP can - // route them through a non-secret transport without going through - // the SM cap. Serialised as base64 to avoid JSON escaping. + // (config.yaml + prompts/ — agent-skills are plugins now, RFC#2843 + // #32) fetched from a non-secret asset channel (template repo / + // Gitea shallow clone per RFC §4.2 transport option (a)). They + // travel on a SEPARATE wire field from ConfigFiles (the SM-bound + // bundle) so a future CP can route them through a non-secret + // transport without going through the SM cap. Serialised as base64 + // to avoid JSON escaping. // // Keys are restricted to the template-asset allowlist enforced by - // IsCPTemplateAssetPath (config.yaml / prompts/* / agent-skills/*); + // IsCPTemplateAssetPath (config.yaml / prompts/* only); // see collectCPConfigFiles for the enforcement. TemplateAssets map[string][]byte `json:"template_assets,omitempty"` } @@ -386,15 +387,15 @@ func (p *CPProvisioner) Start(ctx context.Context, cfg WorkspaceConfig) (string, const cpConfigFilesMaxBytes = 256 << 10 // cpTemplateAssetsMaxBytes bounds the aggregate TEMPLATE-ASSET payload -// (config.yaml + prompts/* + agent-skills/*) delivered via the generic -// non-secret asset channel (RFC #2843 #24). This is a SEPARATE, much larger -// bound than cpConfigFilesMaxBytes: the config bundle is capped at 256 KiB -// because it rides the Secrets-Manager/user-data transport, but template -// ASSETS ride a non-secret channel with no SM size limit — so reusing the -// 256 KiB cap here would re-create the original #2831 skill-drop failure (the -// seo-all skill package alone is ~716 KiB). 16 MiB comfortably fits real skill -// trees + prompts + config while still bounding a runaway/malicious fetcher -// (pure transport-DoS guard, not a secrets-transport limit). +// (config.yaml + prompts/* — agent-skills are NO LONGER carried here per +// RFC#2843 #32; they are plugins installed post-online) delivered via the +// generic non-secret asset channel (RFC #2843 #24). This is a SEPARATE bound +// from cpConfigFilesMaxBytes: the config bundle is capped at 256 KiB because +// it rides the Secrets-Manager/user-data transport, while template ASSETS +// ride a non-secret channel. With skills removed the legitimate payload is +// tiny (config.yaml ~8 KiB + prompts ~8 KiB), but we keep a generous 16 MiB +// bound as a pure transport-DoS guard (not a secrets-transport limit) so a +// runaway/malicious fetcher can't stream an unbounded body. const cpTemplateAssetsMaxBytes = 16 << 20 // isCPTemplateConfigFile restricts which files from a template directory are @@ -432,14 +433,18 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, map[string][] // Blast-radius guard (RC #11690): a fetcher that returns // paths outside the template-asset namespace is either // a programming error or an attack. Either way, fail closed. - // Specifically excluded: MEMORY.md, USER.md, CLAUDE.md - // (curated durable memory — agent-owned state, reconciled by - // the boot entrypoint, NOT by this collect path), .claude/sessions/ - // (Claude Code session dir, agent-owned), and any other - // agent-state path. The PM-flagged in #24_clarify invariant - // is enforced here in code, not in comments. + // Specifically excluded: agent-skills/* (RFC#2843 #32 — skills + // are plugins now, installed post-online, NOT carried on this + // provisioning-time channel; keeping them here re-created the + // #2831/#32 716 KiB skill-tree-in-provision-payload failure), + // MEMORY.md, USER.md, CLAUDE.md (curated durable memory — + // agent-owned state, reconciled by the boot entrypoint, NOT by + // this collect path), .claude/sessions/ (Claude Code session + // dir, agent-owned), and any other agent-state path. The + // PM-flagged in #24_clarify invariant is enforced here in code, + // not in comments. if !IsCPTemplateAssetPath(name) { - return fmt.Errorf("template asset path %q rejected: not in template-asset allowlist (config.yaml / prompts/* / agent-skills/*) — see IsCPTemplateAssetPath", name) + return fmt.Errorf("template asset path %q rejected: not in template-asset allowlist (config.yaml / prompts/* only — agent-skills are plugins now, RFC#2843 #32) — see IsCPTemplateAssetPath", name) } totalAssets += len(data) if totalAssets > cpTemplateAssetsMaxBytes { @@ -508,24 +513,29 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, map[string][] // RFC #2843 #24 — generic template-asset channel. When // cfg.TemplateAssetFetcher is wired (SaaS) and // cfg.TemplateIdentity is set, fetch the template's - // config.yaml + prompts/ + agent_skills/ via the - // non-secret asset channel (template repo, Gitea shallow - // clone per §4.2 transport option (a)) and land them in - // the SEPARATE TemplateAssets field of the provision - // request — NOT in ConfigFiles (the SM-bound bundle). - // The split is load-bearing: ConfigFiles is the bundle - // the CP stages through AWS Secrets Manager, which is - // sized + scoped for *secrets* and caps at 256 KiB. The - // 716 KiB SEO skill package can't ride SM and shouldn't - // try to (core-devops 10:13 SM-inventory RCA). + // config.yaml + prompts/ via the non-secret asset channel + // (template repo, Gitea shallow clone per §4.2 transport + // option (a)) and land them in the SEPARATE TemplateAssets + // field of the provision request — NOT in ConfigFiles (the + // SM-bound bundle). The split is load-bearing: ConfigFiles is + // the bundle the CP stages through AWS Secrets Manager, which + // is sized + scoped for *secrets* and caps at 256 KiB. + // + // agent-skills are NOT fetched here anymore (RFC#2843 #32): + // skills are plugins, installed post-online via the plugin + // pipeline (gitea:// resolver reads agent-skills/ from + // the template repo at install time). The asset payload is + // therefore tiny (config.yaml + prompts), well under any cap — + // this is the #32 fix for fresh-seo-agent provision failing + // closed on a 716 KiB skill tree. // // The fetch is OPT-IN: nil fetcher = no-op (self-host // default; falls through to the local TemplatePath path // above). Every key in the fetcher's output is gated by // IsCPTemplateAssetPath at the addAsset boundary above — - // paths outside the template-asset namespace abort the - // provision rather than silently sneaking MEMORY.md / - // CLAUDE.md / .claude/sessions/ into the bundle. + // paths outside the template-asset namespace (agent-skills/*, + // MEMORY.md, CLAUDE.md, .claude/sessions/) abort the provision + // rather than silently sneaking into the bundle. // // The fetch is fail-closed: a transport error aborts // the provision rather than regressing to stub-mode diff --git a/workspace-server/internal/provisioner/cp_provisioner_test.go b/workspace-server/internal/provisioner/cp_provisioner_test.go index 20169f0c..b5afd193 100644 --- a/workspace-server/internal/provisioner/cp_provisioner_test.go +++ b/workspace-server/internal/provisioner/cp_provisioner_test.go @@ -447,11 +447,12 @@ func TestStart_CollectsConfigFiles(t *testing.T) { // the Start → cpProvisionRequest path end-to-end so a future // refactor that re-merges the two transports would be caught. func TestStart_SendsTemplateAssetsOnSeparateField(t *testing.T) { + // RFC#2843 #32: agent-skills are plugins now and are NOT carried on the + // asset channel — the fetcher emits only config.yaml + prompts/*. prov := &fakeTemplateAssetFetcher{ bundle: map[string][]byte{ - "config.yaml": []byte("# from template repo"), - "prompts/system.md": []byte("# template system prompt"), - "agent-skills/seo-audit/SKILL.md": []byte("# 716 KiB SEO skill goes here"), + "config.yaml": []byte("# from template repo"), + "prompts/system.md": []byte("# template system prompt"), }, } @@ -465,11 +466,11 @@ func TestStart_SendsTemplateAssetsOnSeparateField(t *testing.T) { p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} _, err := p.Start(context.Background(), WorkspaceConfig{ - WorkspaceID: "ws-1", - Runtime: "claude-code", - Tier: 2, - PlatformURL: "http://tenant", - TemplateIdentity: "seo-agent-v1.2.3", + WorkspaceID: "ws-1", + Runtime: "claude-code", + Tier: 2, + PlatformURL: "http://tenant", + TemplateIdentity: "seo-agent-v1.2.3", TemplateAssetFetcher: prov, // No cfg.ConfigFiles — pure fetcher path. If the wire // shape is right, TemplateAssets is non-empty and @@ -479,8 +480,9 @@ func TestStart_SendsTemplateAssetsOnSeparateField(t *testing.T) { t.Fatalf("Start: %v", err) } - // 1. TemplateAssets must contain the fetched files. - if got, want := len(gotBody.TemplateAssets), 3; got != want { + // 1. TemplateAssets must contain the fetched files (config.yaml + prompts + // only — agent-skills are plugins now, RFC#2843 #32). + if got, want := len(gotBody.TemplateAssets), 2; got != want { t.Errorf("TemplateAssets length = %d, want %d (fetcher output must reach the wire)", got, want) } if got, want := string(gotBody.TemplateAssets["config.yaml"]), "# from template repo"; got != want { @@ -489,9 +491,6 @@ func TestStart_SendsTemplateAssetsOnSeparateField(t *testing.T) { if got, want := string(gotBody.TemplateAssets["prompts/system.md"]), "# template system prompt"; got != want { t.Errorf("TemplateAssets[prompts/system.md] = %q, want %q", got, want) } - if got, want := string(gotBody.TemplateAssets["agent-skills/seo-audit/SKILL.md"]), "# 716 KiB SEO skill goes here"; got != want { - t.Errorf("TemplateAssets[agent-skills/seo-audit/SKILL.md] = %q, want %q", got, want) - } // 2. ConfigFiles must be empty — the transport split. if got, want := len(gotBody.ConfigFiles), 0; got != want { @@ -520,11 +519,11 @@ func TestStart_AbortsOnFetcherAssetOutsideAllowlist(t *testing.T) { p := &CPProvisioner{baseURL: srv.URL, orgID: "org-1", httpClient: srv.Client()} _, err := p.Start(context.Background(), WorkspaceConfig{ - WorkspaceID: "ws-1", - Runtime: "claude-code", - Tier: 2, - PlatformURL: "http://tenant", - TemplateIdentity: "seo-agent-v1.2.3", + WorkspaceID: "ws-1", + Runtime: "claude-code", + Tier: 2, + PlatformURL: "http://tenant", + TemplateIdentity: "seo-agent-v1.2.3", TemplateAssetFetcher: prov, }) if err == nil { diff --git a/workspace-server/internal/provisioner/gitea_template_assets.go b/workspace-server/internal/provisioner/gitea_template_assets.go index dc92c11f..7e8771dd 100644 --- a/workspace-server/internal/provisioner/gitea_template_assets.go +++ b/workspace-server/internal/provisioner/gitea_template_assets.go @@ -7,8 +7,9 @@ package provisioner // Transport: GET {baseURL}/api/v1/repos///archive/.tar.gz // with header "Authorization: token " → stream-extract the // tarball → return map[relpath][]byte for ONLY allowlisted paths -// (config.yaml + prompts/** + agent-skills/**) → strip the archive's -// top-level dir prefix. +// (config.yaml + prompts/** — agent-skills are plugins now, RFC#2843 +// #32, and are skipped by the IsCPTemplateAssetPath filter) → strip +// the archive's top-level dir prefix. // // Template identity format: "/@" (e.g. // "molecule-ai/workspace-template-claude-code@main"). The caller @@ -22,14 +23,16 @@ package provisioner // persisted-bundle provider in #2831 PIECE 1). // // Memory-preservation: this fetcher ONLY materializes TEMPLATE -// ASSETS (config.yaml + prompts/* + agent-skills/*). The existing -// IsCPTemplateAssetPath allowlist in the consumer (collectCPConfigFiles) -// is the load-bearing guard against a fetcher that returns a path -// outside the template-asset namespace (e.g. /workspace, MEMORY.md, -// USER.md, CLAUDE.md, .claude/sessions/*) — those would either be -// rejected by the allowlist (provision aborts) or, if they somehow -// slipped through, would land in the SEPARATE TemplateAssets field -// rather than the SM-bound ConfigFiles field. The transport split +// ASSETS (config.yaml + prompts/*). agent-skills/* are NOT carried +// (RFC#2843 #32 — skills are plugins now, installed post-online); the +// IsCPTemplateAssetPath filter below skips them. That same allowlist +// in the consumer (collectCPConfigFiles) is the load-bearing guard +// against a fetcher that returns a path outside the template-asset +// namespace (e.g. agent-skills/*, /workspace, MEMORY.md, USER.md, +// CLAUDE.md, .claude/sessions/*) — those would either be rejected by +// the allowlist (provision aborts) or, if they somehow slipped +// through, would land in the SEPARATE TemplateAssets field rather +// than the SM-bound ConfigFiles field. The transport split // (TemplateAssets vs ConfigFiles) is the second line of defense // against clobbering agent-owned state. // @@ -221,7 +224,10 @@ func (f *giteaTemplateAssetFetcher) Load(ctx context.Context, templateIdentity s // (collectCPConfigFiles) ALSO gates on IsCPTemplateAssetPath; // skipping non-allowlisted entries here is a free perf win // (don't allocate bytes for paths the consumer will reject) - // and a cleaner audit log. + // and a cleaner audit log. Per RFC#2843 #32 this also skips the + // (potentially large) agent-skills/* tree — skills are plugins + // now, fetched at install time by the gitea:// plugin resolver, + // NOT carried on this provisioning-time channel. if !IsCPTemplateAssetPath(rel) { continue } diff --git a/workspace-server/internal/provisioner/gitea_template_assets_test.go b/workspace-server/internal/provisioner/gitea_template_assets_test.go index 8092a181..abcf4dee 100644 --- a/workspace-server/internal/provisioner/gitea_template_assets_test.go +++ b/workspace-server/internal/provisioner/gitea_template_assets_test.go @@ -8,9 +8,10 @@ package provisioner // Tests use httptest.NewServer to serve a real .tar.gz // generated in-memory (no real Gitea instance needed). The // dispatch's required test surface: -// - happy path: assert ALL asset paths incl agent-skills are -// returned (must FAIL if skills dropped) -// - allowlist filter: non-allowlisted paths are excluded +// - happy path: assert config.yaml + prompts/* are returned and +// agent-skills/* are SKIPPED (RFC#2843 #32 — skills are plugins +// now, fetched at install time, NOT on this asset channel) +// - allowlist filter: non-allowlisted paths (incl agent-skills) excluded // - fail-closed: transport / extract errors surface as errors // - identity parsing: malformed identities return errors @@ -29,10 +30,12 @@ import ( // TestGiteaTemplateAssetFetcher_HappyPath pins the production // contract: a real .tar.gz archive containing config.yaml + -// prompts/ + agent-skills/ is fetched, parsed, and returned -// as a map with all three namespaces populated. The dispatch -// explicitly calls out: "must FAIL if skills dropped" — this -// test is the load-bearing check for that. +// prompts/ + agent-skills/ is fetched and parsed; config.yaml + +// prompts/* are returned, and agent-skills/* are SKIPPED (RFC#2843 +// #32 — skills are plugins now, fetched at install time by the +// gitea:// plugin resolver, NOT carried on this asset channel). The +// skill files MUST still exist in the repo archive — they are the +// plugin source — they are just not asset-delivered. func TestGiteaTemplateAssetFetcher_HappyPath(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Verify the request shape. @@ -43,8 +46,10 @@ func TestGiteaTemplateAssetFetcher_HappyPath(t *testing.T) { t.Errorf("unexpected Authorization header: %q", got) } // Serve a real .tar.gz with config.yaml + prompts/system.md + - // agent-skills/seo-audit/SKILL.md wrapped in the - // "-" top-level dir Gitea uses. + // agent-skills/seo-audit/* wrapped in the "-" + // top-level dir Gitea uses. The skill files are present in the + // archive (they ARE the plugin source) but must be SKIPPED by + // the fetcher (RFC#2843 #32). w.Header().Set("Content-Type", "application/gzip") w.WriteHeader(http.StatusOK) gz := gzip.NewWriter(w) @@ -64,15 +69,15 @@ func TestGiteaTemplateAssetFetcher_HappyPath(t *testing.T) { t.Fatalf("Load: %v", err) } - // All 3 allowlisted namespaces must be present. Per the - // dispatch: "must FAIL if skills dropped" — assert skill - // files explicitly. + // config.yaml + prompts/* are returned; agent-skills/* are SKIPPED + // (RFC#2843 #32 — skills are plugins now). The skill files exist in + // the repo archive but must NOT appear in the asset map. mustHaveKey(t, assets, "config.yaml") mustHaveKey(t, assets, "prompts/system.md") - mustHaveKey(t, assets, "agent-skills/seo-audit/SKILL.md") - mustHaveKey(t, assets, "agent-skills/seo-audit/manifest.yaml") - if len(assets) != 4 { - t.Errorf("expected 4 assets, got %d: %v", len(assets), keysOf(assets)) + mustNotHaveKey(t, assets, "agent-skills/seo-audit/SKILL.md") + mustNotHaveKey(t, assets, "agent-skills/seo-audit/manifest.yaml") + if len(assets) != 2 { + t.Errorf("expected 2 assets (config.yaml + prompts/system.md), got %d: %v", len(assets), keysOf(assets)) } } @@ -91,8 +96,9 @@ func TestGiteaTemplateAssetFetcher_AllowsOnlyAllowlistedPaths(t *testing.T) { // Allowlisted. mustWriteTar(t, tw, "repo-sha/config.yaml", []byte("ok")) mustWriteTar(t, tw, "repo-sha/prompts/x.md", []byte("ok")) - mustWriteTar(t, tw, "repo-sha/agent-skills/skill-x/SKILL.md", []byte("ok")) - // NOT allowlisted — must be excluded. + // NOT allowlisted — must be excluded. agent-skills/* are plugins + // now (RFC#2843 #32) and are no longer asset-eligible. + mustWriteTar(t, tw, "repo-sha/agent-skills/skill-x/SKILL.md", []byte("plugin-source-not-asset")) mustWriteTar(t, tw, "repo-sha/CLAUDE.md", []byte("agent-owned")) mustWriteTar(t, tw, "repo-sha/MEMORY.md", []byte("agent-owned")) mustWriteTar(t, tw, "repo-sha/USER.md", []byte("agent-owned")) @@ -112,15 +118,15 @@ func TestGiteaTemplateAssetFetcher_AllowsOnlyAllowlistedPaths(t *testing.T) { // Allowlisted keys present. mustHaveKey(t, assets, "config.yaml") mustHaveKey(t, assets, "prompts/x.md") - mustHaveKey(t, assets, "agent-skills/skill-x/SKILL.md") - // Non-allowlisted keys EXCLUDED. + // Non-allowlisted keys EXCLUDED — incl agent-skills/* (RFC#2843 #32). + mustNotHaveKey(t, assets, "agent-skills/skill-x/SKILL.md") mustNotHaveKey(t, assets, "CLAUDE.md") mustNotHaveKey(t, assets, "MEMORY.md") mustNotHaveKey(t, assets, "USER.md") mustNotHaveKey(t, assets, ".claude/sessions/foo.json") mustNotHaveKey(t, assets, "adapter.py") - if len(assets) != 3 { - t.Errorf("expected 3 allowlisted assets, got %d: %v", len(assets), keysOf(assets)) + if len(assets) != 2 { + t.Errorf("expected 2 allowlisted assets, got %d: %v", len(assets), keysOf(assets)) } } @@ -237,6 +243,8 @@ func TestGiteaTemplateAssetFetcher_EmptyToken_RealHTTP_NoAuthHeader_Success(t *t // not just a minimal 1-asset happy path. mustWriteTar(t, tw, "repo-sha/config.yaml", []byte("# public-template config\n")) mustWriteTar(t, tw, "repo-sha/prompts/system.md", []byte("# public-template system prompt\n")) + // agent-skills present in the repo but SKIPPED by the fetcher + // (RFC#2843 #32 — plugins, not assets). mustWriteTar(t, tw, "repo-sha/agent-skills/skill-x/SKILL.md", []byte("# public-template skill\n")) _ = tw.Close() _ = gz.Close() @@ -250,9 +258,9 @@ func TestGiteaTemplateAssetFetcher_EmptyToken_RealHTTP_NoAuthHeader_Success(t *t } mustHaveKey(t, assets, "config.yaml") mustHaveKey(t, assets, "prompts/system.md") - mustHaveKey(t, assets, "agent-skills/skill-x/SKILL.md") - if len(assets) != 3 { - t.Errorf("expected 3 assets, got %d: %v", len(assets), keysOf(assets)) + mustNotHaveKey(t, assets, "agent-skills/skill-x/SKILL.md") + if len(assets) != 2 { + t.Errorf("expected 2 assets (config.yaml + prompts/system.md), got %d: %v", len(assets), keysOf(assets)) } } diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index bb26fc92..8cf1021c 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -110,21 +110,21 @@ const ( // WorkspaceConfig holds the parameters needed to provision a workspace container. type WorkspaceConfig struct { - WorkspaceID string - TemplatePath string // Host path to template dir to copy from (e.g. claude-code-default/) - TemplateIdentity string // RFC #2843 #24: opaque token the TemplateAssetFetcher resolves to the template repo+ref (e.g. "claudius-v1.2.3" or a sha). Used by SaaS; ignored by the local-dir TemplatePath path. - ConfigFiles map[string][]byte // Generated config files to write into /configs volume - PluginsPath string // Host path to plugins directory (mounted at /plugins) - WorkspacePath string // Host path to bind-mount as /workspace (if empty, uses Docker named volume) - Tier int - Runtime string // "claude-code" (default), "codex", "hermes", "openclaw", etc. - InstanceType string // Optional CP EC2 instance type override (SaaS only) - DiskGB int32 // Optional CP root volume size override in GiB (SaaS only) - DataPersistence string // internal#734: "persist"|"ephemeral"|"" — durable-data choice forwarded to CP (SaaS only) - Provider string // multi-provider RFC: ""/"aws"|"hetzner"|"gcp" compute backend for the workspace box (per-workspace; distinct from LLM/model provider). Forwarded to CP. - Display WorkspaceDisplayConfig - EnvVars map[string]string // Additional env vars (API keys, etc.) - PlatformURL string + WorkspaceID string + TemplatePath string // Host path to template dir to copy from (e.g. claude-code-default/) + TemplateIdentity string // RFC #2843 #24: opaque token the TemplateAssetFetcher resolves to the template repo+ref (e.g. "claudius-v1.2.3" or a sha). Used by SaaS; ignored by the local-dir TemplatePath path. + ConfigFiles map[string][]byte // Generated config files to write into /configs volume + PluginsPath string // Host path to plugins directory (mounted at /plugins) + WorkspacePath string // Host path to bind-mount as /workspace (if empty, uses Docker named volume) + Tier int + Runtime string // "claude-code" (default), "codex", "hermes", "openclaw", etc. + InstanceType string // Optional CP EC2 instance type override (SaaS only) + DiskGB int32 // Optional CP root volume size override in GiB (SaaS only) + DataPersistence string // internal#734: "persist"|"ephemeral"|"" — durable-data choice forwarded to CP (SaaS only) + Provider string // multi-provider RFC: ""/"aws"|"hetzner"|"gcp" compute backend for the workspace box (per-workspace; distinct from LLM/model provider). Forwarded to CP. + Display WorkspaceDisplayConfig + EnvVars map[string]string // Additional env vars (API keys, etc.) + PlatformURL string // WorkspaceSecretKeys are env keys authored via the workspace_secrets table // (user/org-admin set, per-workspace). The Forensic #145 SCM-write-token @@ -137,7 +137,8 @@ type WorkspaceConfig struct { // TemplateAssetFetcher (RFC #2843 #24) is the generic // non-secret asset channel for template assets - // (config.yaml + prompts/ + agent-skills/). The fetcher + // (config.yaml + prompts/ — agent-skills are plugins now, + // RFC#2843 #32, and are not carried here). The fetcher // resolves cfg.TemplateIdentity to a shallow clone of the // template repo (Gitea per RFC §4.2 transport option (a)) // and returns the asset file map. nil = no provider wired @@ -282,7 +283,7 @@ type dockerClient interface { // Provisioner manages Docker containers for workspace agents. type Provisioner struct { - cli dockerClient + cli dockerClient alpineImage string // overridable in tests; production uses the digest-pinned default } @@ -593,7 +594,6 @@ func buildStartWorkspaceEnv(cfg WorkspaceConfig, hostPort string) []string { fmt.Sprintf("MOLECULE_WORKSPACE_URL=%s", workspaceAdvertiseURL(hostPort))) } - // resolveStartWorkspaceHostURL computes the hostURL that StartWorkspace // should return to the platform. The host comes from workspaceAdvertiseURL // (env override → localhost); the port starts at hostPort and is swapped diff --git a/workspace-server/internal/provisioner/template_assets.go b/workspace-server/internal/provisioner/template_assets.go index 019414c4..d2d42608 100644 --- a/workspace-server/internal/provisioner/template_assets.go +++ b/workspace-server/internal/provisioner/template_assets.go @@ -3,12 +3,17 @@ package provisioner // template_assets.go — generic template-asset channel (RFC #2843 #24). // // This is the "generic, non-secret asset channel" the RFC proposes: -// a workspace's template assets (config.yaml + prompts/ + agent-skills/) -// are materialized from a template identity (resolved by the caller — +// a workspace's template assets (config.yaml + prompts/) are +// materialized from a template identity (resolved by the caller — // the template repo path, a cached ref, etc.) rather than forced // through the AWS Secrets Manager config bundle path that caps at -// cpConfigFilesMaxBytes (256 KiB) and silently drops any skill over -// the cap. +// cpConfigFilesMaxBytes (256 KiB). +// +// agent-skills are NO LONGER carried on this channel (RFC#2843 #32): +// skills are PLUGINS now, installed dynamically post-online via the +// plugin pipeline (the gitea:// plugin resolver reads the +// agent-skills/ subpath from the template repo at install +// time). See IsCPTemplateAssetPath for the load-bearing allowlist. // // The fetcher is interface-typed so tests inject fakes and the real // implementation (in main.go) wires the Gitea shallow-clone per @@ -48,14 +53,15 @@ import ( ) // TemplateAssetFetcher materializes a template's -// config.yaml + prompts/ + agent-skills/ from a non-secret -// asset channel (template repo, Gitea shallow clone per -// RFC #2843 §4.2). Returned paths are RELATIVE to the -// template asset root (e.g. "config.yaml", "prompts/system.md", -// "agent-skills/seo-audit/SKILL.md") and the bytes are raw +// config.yaml + prompts/ from a non-secret asset channel +// (template repo, Gitea shallow clone per RFC #2843 §4.2). +// Returned paths are RELATIVE to the template asset root (e.g. +// "config.yaml", "prompts/system.md") and the bytes are raw // file contents (not base64-encoded — the generic channel // does not require encoding; the wire format encodes per -// its own transport). +// its own transport). agent-skills/* paths are NOT eligible +// (RFC#2843 #32 — skills are plugins now; see IsCPTemplateAssetPath) +// and are rejected at the consumer boundary if returned. // // Returned errors: a transport / resolution failure is // returned as a non-nil error so the caller can abort the @@ -79,14 +85,28 @@ type TemplateAssetFetcher interface { // // - "config.yaml" — the runtime entrypoint config // - "prompts/*" — system prompts -// - "agent-skills/*" — the agent's skill packages // // Everything else is REJECTED. Specifically excluded: -// MEMORY.md / USER.md (curated durable memory — agent-owned -// state, reconciled by the boot entrypoint, not by this -// collect path), CLAUDE.md (runtime memory file, agent-owned), -// .claude/sessions/* (Claude Code session dir, agent-owned), -// anything outside the template-asset namespace. +// - "agent-skills/*" — agent skills are NO LONGER asset-channel +// eligible (RFC#2843 #32). Skills are PLUGINS now: they install +// DYNAMICALLY after the workspace boots online, via the plugin +// install pipeline (gitea:// plugin source → the reconcile reads +// the agent-skills/ subpath from the template repo at +// INSTALL time), NOT through this provisioning-time asset channel. +// Keeping agent-skills/* in this allowlist re-created the original +// #2831 failure: the ~716 KiB seo-all skill tree got pulled into +// the provision request, which fail-closed BEFORE the CP was ever +// called (the asset payload no longer rides SM, but the skills +// have no business in the provision payload at all now that they +// are plugins). The skill files MUST remain in the template repo +// (the gitea:// plugin resolver reads them at install time) — this +// allowlist only governs what the PROVISION-TIME asset channel +// carries. +// - MEMORY.md / USER.md (curated durable memory — agent-owned state, +// reconciled by the boot entrypoint, not by this collect path), +// - CLAUDE.md (runtime memory file, agent-owned), +// - .claude/sessions/* (Claude Code session dir, agent-owned), +// - anything outside the template-asset namespace. // // Path normalization: the function applies filepath.ToSlash // + filepath.Clean before matching, so Windows-style @@ -100,8 +120,7 @@ type TemplateAssetFetcher interface { func IsCPTemplateAssetPath(name string) bool { name = filepath.ToSlash(filepath.Clean(name)) return name == "config.yaml" || - strings.HasPrefix(name, "prompts/") || - strings.HasPrefix(name, "agent-skills/") + strings.HasPrefix(name, "prompts/") } // noopTemplateAssetFetcher is the self-host default fetcher (PR-B diff --git a/workspace-server/internal/provisioner/template_assets_test.go b/workspace-server/internal/provisioner/template_assets_test.go index 5c1fe4ca..c06bd2c2 100644 --- a/workspace-server/internal/provisioner/template_assets_test.go +++ b/workspace-server/internal/provisioner/template_assets_test.go @@ -11,9 +11,9 @@ package provisioner // SM-bound ConfigFiles). // 3. Every key in the fetcher's output is gated by // IsCPTemplateAssetPath. Paths outside the template-asset -// allowlist (config.yaml / prompts/* / agent-skills/*) ABORT -// the provision — Reviewer-CR2 RC #11690's load-bearing -// blast-radius guard. +// allowlist (config.yaml / prompts/* only — agent-skills are +// plugins now, RFC#2843 #32) ABORT the provision — Reviewer-CR2 +// RC #11690's load-bearing blast-radius guard. // 4. A transport error on the fetcher ABORTS the provision // (fail-closed; never regresses to stub /configs). // 5. Nil fetcher = no-op (self-host default; the existing @@ -55,9 +55,8 @@ func (f *fakeTemplateAssetFetcher) Load(_ context.Context, templateIdentity stri func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { prov := &fakeTemplateAssetFetcher{ bundle: map[string][]byte{ - "config.yaml": []byte("# from template repo"), - "prompts/system.md": []byte("# template system prompt"), - "agent-skills/seo-audit/SKILL.md": []byte("# seo skill"), + "config.yaml": []byte("# from template repo"), + "prompts/system.md": []byte("# template system prompt"), }, } cfg := WorkspaceConfig{ @@ -69,13 +68,15 @@ func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { if err != nil { t.Fatalf("collectCPConfigFiles: %v", err) } - // All 3 fetched assets land in TemplateAssets (the + // The fetched assets land in TemplateAssets (the // non-secret transport), NOT in ConfigFiles (the SM-bound // bundle). The transport split is the load-bearing fix. + // NB (RFC#2843 #32): agent-skills/* are NOT carried on this + // channel anymore — skills are plugins. Only config.yaml + + // prompts/* are asset-eligible. wantKeys := []string{ "config.yaml", "prompts/system.md", - "agent-skills/seo-audit/SKILL.md", } for _, wk := range wantKeys { if _, ok := assets[wk]; !ok { @@ -91,6 +92,33 @@ func TestCollectCPConfigFiles_MergesFetcherAssets(t *testing.T) { } } +// TestCollectCPConfigFiles_RejectsAgentSkillsAsset is the RFC#2843 #32 +// regression: a fetcher that returns an agent-skills/* path must ABORT the +// provision (skills are plugins now, NOT asset-channel eligible). Before #32 +// the allowlist admitted agent-skills/* and the ~716 KiB seo-all tree got +// pulled into the provision payload, which fail-closed BEFORE the CP was ever +// called. Guards against re-adding agent-skills/* to IsCPTemplateAssetPath. +func TestCollectCPConfigFiles_RejectsAgentSkillsAsset(t *testing.T) { + prov := &fakeTemplateAssetFetcher{ + bundle: map[string][]byte{ + "config.yaml": []byte("# from template repo"), + "agent-skills/seo-audit/SKILL.md": []byte("# 716 KiB skill tree does not belong here"), + }, + } + cfg := WorkspaceConfig{ + TemplateIdentity: "seo-agent-v1.2.3", + TemplateAssetFetcher: prov, + } + + _, _, err := collectCPConfigFiles(cfg) + if err == nil { + t.Fatal("expected collectCPConfigFiles to abort when fetcher returns an agent-skills/* path, got nil") + } + if !stringsContains(err.Error(), "allowlist") { + t.Errorf("expected the reject error to mention the allowlist, got: %v", err) + } +} + // TestCollectCPConfigFiles_CallerWinsOnConfigFiles asserts // the caller's cfg.ConfigFiles entry lands in the ConfigFiles // field (the SM-bound bundle) even when a fetcher is wired. @@ -138,9 +166,9 @@ func TestCollectCPConfigFiles_CallerWinsOnConfigFiles(t *testing.T) { // /configs (the same fail-closed contract as the // persisted-bundle provider in #2831 PIECE 1). If a future // refactor swallows the fetch error, the bundle would -// silently miss the agent-skills/ files and the workspace -// would boot with a stub config — the exact regression -// this test guards against. +// silently miss the config.yaml + prompts files and the +// workspace would boot with a stub config — the exact +// regression this test guards against. func TestCollectCPConfigFiles_FetcherErrorAborts(t *testing.T) { prov := &fakeTemplateAssetFetcher{err: errors.New("gitea 503")} cfg := WorkspaceConfig{ @@ -221,17 +249,22 @@ func TestIsCPTemplateAssetPath_AllowsPromptsPrefix(t *testing.T) { } } -// TestIsCPTemplateAssetPath_AllowsAgentSkillsPrefix pins -// the agent-skills/* namespace (the load-bearing addition -// for the 716 KiB SEO skill package per core-devops 10:13). -func TestIsCPTemplateAssetPath_AllowsAgentSkillsPrefix(t *testing.T) { - for _, ok := range []string{ +// TestIsCPTemplateAssetPath_RejectsAgentSkillsPrefix pins the RFC#2843 #32 +// contract: agent-skills/* is NO LONGER asset-channel eligible. Skills are +// PLUGINS now — they install dynamically post-online via the plugin pipeline +// (the gitea:// resolver reads agent-skills/ from the template repo at +// install time), NOT through the provisioning-time asset channel. Keeping +// agent-skills/* in the allowlist re-created the #32 fresh-seo-agent provision +// failure (the ~716 KiB seo-all tree pulled into the provision payload). +func TestIsCPTemplateAssetPath_RejectsAgentSkillsPrefix(t *testing.T) { + for _, bad := range []string{ "agent-skills/seo-audit/SKILL.md", "agent-skills/seo-audit/manifest.yaml", "agent-skills/index.json", + "agent-skills/seo-all/SKILL.md", } { - if !IsCPTemplateAssetPath(ok) { - t.Errorf("expected %q to be allowed", ok) + if IsCPTemplateAssetPath(bad) { + t.Errorf("expected %q to be REJECTED (agent-skills are plugins now, RFC#2843 #32)", bad) } } } @@ -392,8 +425,8 @@ func TestCollectCPConfigFiles_RejectsFetcherAssetOutsideAllowlist(t *testing.T) func TestCollectCPConfigFiles_FetcherAssetsRawBytes(t *testing.T) { prov := &fakeTemplateAssetFetcher{ bundle: map[string][]byte{ - "config.yaml": []byte("# raw bytes, will be base64 by marshaler"), - "agent-skills/seo-audit/SKILL.md": []byte("raw-skill"), + "config.yaml": []byte("# raw bytes, will be base64 by marshaler"), + "prompts/seo-agent.md": []byte("raw-prompt"), }, } cfg := WorkspaceConfig{ @@ -407,8 +440,8 @@ func TestCollectCPConfigFiles_FetcherAssetsRawBytes(t *testing.T) { if got := string(assets["config.yaml"]); got != "# raw bytes, will be base64 by marshaler" { t.Errorf("expected raw bytes in TemplateAssets, got %q (encoding happens at marshal time, not at collect time)", got) } - if got := string(assets["agent-skills/seo-audit/SKILL.md"]); got != "raw-skill" { - t.Errorf("expected raw skill bytes in TemplateAssets, got %q", got) + if got := string(assets["prompts/seo-agent.md"]); got != "raw-prompt" { + t.Errorf("expected raw prompt bytes in TemplateAssets, got %q", got) } } @@ -480,23 +513,28 @@ func stringsContains(s, substr string) bool { return strings.Contains(s, substr) } -// TestCollectCPConfigFiles_AssetsAllowLargeSkillPackage is the regression for +// TestCollectCPConfigFiles_AssetsNotBoundBy256KCap is the regression for // Reviewer-CR2's size-cap RC on head 7bcc3b5f: template ASSETS ride a // NON-secret channel and must NOT be bound by the 256 KiB SM/config-bundle cap -// (cpConfigFilesMaxBytes). Reusing that cap re-creates the original #2831 -// skill-drop — the motivating 716 KiB seo-all package would abort client-side. -// A >256 KiB asset payload must SUCCEED on TemplateAssets (bounded only by the -// far larger cpTemplateAssetsMaxBytes DoS guard) while ConfigFiles stays capped. -func TestCollectCPConfigFiles_AssetsAllowLargeSkillPackage(t *testing.T) { - // 716 KiB skill blob — over the old 256 KiB cap, well under the asset bound. +// (cpConfigFilesMaxBytes). A >256 KiB asset payload must SUCCEED on +// TemplateAssets (bounded only by the far larger cpTemplateAssetsMaxBytes DoS +// guard) while ConfigFiles stays capped. +// +// NB (RFC#2843 #32): agent-skills are NO LONGER asset-eligible (skills are +// plugins now), so this uses a large prompts/* asset — the largest legitimate +// asset payload after the skill tree was removed from the channel. In practice +// the seo-agent asset set is tiny (config.yaml ~8 KiB + prompts ~8 KiB); this +// test only pins that the asset cap is the larger one, not the SM cap. +func TestCollectCPConfigFiles_AssetsNotBoundBy256KCap(t *testing.T) { + // 716 KiB prompt blob — over the old 256 KiB cap, well under the asset bound. big := make([]byte, 716<<10) for i := range big { big[i] = 'x' } prov := &fakeTemplateAssetFetcher{ bundle: map[string][]byte{ - "config.yaml": []byte("# from template repo"), - "agent-skills/seo-audit/big-skill.md": big, + "config.yaml": []byte("# from template repo"), + "prompts/big-prompt.md": big, }, } cfg := WorkspaceConfig{ @@ -506,10 +544,10 @@ func TestCollectCPConfigFiles_AssetsAllowLargeSkillPackage(t *testing.T) { _, assets, err := collectCPConfigFiles(cfg) if err != nil { - t.Fatalf("a %d-byte skill package must succeed on the non-secret asset channel (no SM cap), got error: %v", len(big), err) + t.Fatalf("a %d-byte prompts asset must succeed on the non-secret asset channel (no SM cap), got error: %v", len(big), err) } - if got := len(assets["agent-skills/seo-audit/big-skill.md"]); got != len(big) { - t.Errorf("expected the full %d-byte skill in TemplateAssets, got %d", len(big), got) + if got := len(assets["prompts/big-prompt.md"]); got != len(big) { + t.Errorf("expected the full %d-byte prompt in TemplateAssets, got %d", len(big), got) } } -- 2.52.0 From b46e0349c2e84510a43b708d0602cf4072ae0be0 Mon Sep 17 00:00:00 2001 From: core-devops Date: Tue, 16 Jun 2026 14:11:37 -0700 Subject: [PATCH 2/2] =?UTF-8?q?RFC#2843=20#32:=20fix=20template-delivery-e?= =?UTF-8?q?2e=20job=20rename=20=E2=80=94=20colon-free=20name=20+=20bp-exem?= =?UTF-8?q?pt=20directive?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The renamed advisory job emits a new status context; lint-required-context-exists-in-bp flagged it. Quote-free colon-free name (PyYAML AST parse rejected the colon) + a bp-exempt directive within the 3-line window above the job key (advisory Phase-1 gate, continue-on-error mc#2996 — not a required BP context). Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/workflows/template-delivery-e2e.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.gitea/workflows/template-delivery-e2e.yml b/.gitea/workflows/template-delivery-e2e.yml index 27dc2567..8f2ee21d 100644 --- a/.gitea/workflows/template-delivery-e2e.yml +++ b/.gitea/workflows/template-delivery-e2e.yml @@ -68,8 +68,14 @@ concurrency: cancel-in-progress: true jobs: + # Job renamed for the RFC#2843 #32 two-channel contract (config+prompts via + # the asset channel; seo-all installs via the post-online plugin reconcile, + # not at boot). Renaming the job changes the emitted status context. + # bp-exempt: advisory Phase-1 gate (continue-on-error, mc#2996) — informational, not a required BP context. delivery: - name: Template-asset delivery (fresh seo-agent: config+prompts via asset channel, seo-all via plugin reconcile) + # No colon in the name — lint-required-context's PyYAML AST parse rejects an + # unquoted scalar containing a colon. + name: Template-asset delivery (fresh seo-agent — config+prompts via asset channel, seo-all via plugin reconcile) runs-on: ubuntu-latest # Phase 1: advisory. Remove this line in Phase 2 to make it merge-blocking. # mc#2996 — Phase 2 promotion tracker (remove continue-on-error; forced 14d renewal cadence). -- 2.52.0