diff --git a/apps/daemon/src/prompts/system.ts b/apps/daemon/src/prompts/system.ts index e9256d08db..439e6d5a50 100644 --- a/apps/daemon/src/prompts/system.ts +++ b/apps/daemon/src/prompts/system.ts @@ -627,6 +627,44 @@ When you write or edit an HTML file in the project folder through the native fil - After the final self-check, briefly name the written file and summarize the result instead. - A filesystem run that emits a source-code \`\` is treated as an unexpected fallback by the host.`; +// Skills are a read-only resource to every agent: `resolveChatExtraAllowedDirs` +// keeps the skill roots out of the writable allowlist, and Codex is given no +// skill directory at all because it treats `--add-dir` as write access (PR +// #622). The daemon stages a private copy into `/.od-skills/` for reading, +// and that copy is a deliberate write barrier — edits to it change nothing. +// +// That staged copy needs its own sentence in the prompt, because it is the one +// skill path where the permission error does NOT fire. `withSkillRootPreamble` +// advertises `.od-skills//` as the primary **Skill root**, and it sits +// inside the agent's writable project cwd — so a `Write`/`Edit`/`Bash` against +// `.od-skills//SKILL.md` succeeds, looks like an install, and is discarded +// when `stageActiveSkill` replaces the copy wholesale on the next turn. Silent +// success is the same misdirection as the invented setting, minus the error. +// +// None of that was ever stated in the prompt. An agent asked to update a skill +// therefore wrote to the skill path, collected `Operation not permitted`, and — +// having no stated exit — invented one, telling the user to add the directory +// to a "writable roots" setting Open Design does not have. The user searched +// for that setting, could not find it, and filed a diagnostics bundle (0.21.0). +// +// So: state the boundary, state the real exit, and forbid inventing product +// settings. The last paragraph is worded as a statement about the current build +// rather than a refusal, because contributors read "we do not have this" as "we +// will not build this", and a controlled skill-write path is on the table. +const SKILL_WRITE_BOUNDARY = ` + +--- + +## Editing skills + +Skill directories are not writable through your file tools, and that is deliberate — a skill is part of your own instructions, so it is never edited as a side effect of a task. Attempting to write one returns \`Operation not permitted\`. Retrying, changing the path, or routing the same write through a shell command will not help. + +The \`.od-skills/\` folder in the project is a private working copy staged for reading only. It is the one exception to the error above: it sits in a directory you can write, so an edit there will appear to succeed while updating nothing, and the copy is replaced on the next turn. Do not edit that path, and never report a successful write there as having created or updated a skill. + +When the user asks you to create or change a skill, do the work and hand it over rather than trying to install it yourself: write the proposal as a new \`.md\` file in the project folder — which you *can* write — never under \`.od-skills/\`, then tell the user to paste it in through the Integration view's Skills tab, which is where skills are edited in the app. + +Open Design does not currently expose a sandbox mode, a writable-directory or "writable roots" list, or an approval-policy setting. Do not tell the user to look for one, and do not invent a settings path, menu, or option name to explain the failure — they will go looking and find nothing. Describe the limitation plainly and point at the Skills tab instead. (If a future build does add such a surface, this paragraph is what should change.)`; + export function buildExamplePromptOverride( title?: string | null, brief?: Record | null, @@ -1425,6 +1463,21 @@ export function composeSystemPrompt({ parts.push(FILESYSTEM_HANDOFF_OVERRIDE); } + // Gated on the execution profile only — deliberately NOT on `isSlimCore` or + // on an active skill. Slim is the production default (`server.ts` sends + // `promptCoreVariant: 'slim'` unless `OD_PROMPT_CORE=classic`), so a + // `!isSlimCore` gate would ship this to nobody; and the request that triggers + // the failure ("update my skill") arrives on turns where no skill is active, + // so gating on `skillBody` would miss the reported case entirely. + // + // `filesystem` is what excludes the runs that cannot hit the boundary: Ask + // mode is bare conversation, and API/BYOK mode (`streamFormat: 'plain'` -> + // `text_artifact`) has no file tools at all, where a paragraph about file + // tools failing would be noise contradicting its "no tools available" head. + if (!isAskMode && resolvedExecutionProfile === 'filesystem') { + parts.push(SKILL_WRITE_BOUNDARY); + } + // Clarification on any turn reuses the same `` flow so the // host keeps ONE unified questions surface: the form renders inline in the // originating assistant message, and answers return as the next user message. diff --git a/apps/daemon/tests/prompts/__snapshots__/system-prompt-matrix.test.ts.snap b/apps/daemon/tests/prompts/__snapshots__/system-prompt-matrix.test.ts.snap index a2b7053123..4ec54f9922 100644 --- a/apps/daemon/tests/prompts/__snapshots__/system-prompt-matrix.test.ts.snap +++ b/apps/daemon/tests/prompts/__snapshots__/system-prompt-matrix.test.ts.snap @@ -45,10 +45,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-generation-contract", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 48156, + "totalChars": 49699, }, "critique-enabled": { "sections": [ @@ -66,10 +67,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "critique-panel", "active-ds-visual-direction-override", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 60919, + "totalChars": 62462, }, "deck-kind-no-skill": { "sections": [ @@ -81,10 +83,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "deck-framework", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 89358, + "totalChars": 90901, }, "deck-kind-with-skill-seed": { "sections": [ @@ -96,10 +99,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 60120, + "totalChars": 61663, }, "design-ds-fixture-fallback": { "sections": [ @@ -117,10 +121,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "media-dispatch-hint", "active-ds-visual-direction-override", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 85440, + "totalChars": 86983, }, "design-full-stack": { "sections": [ @@ -147,10 +152,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "media-dispatch-hint", "active-ds-visual-direction-override", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 70255, + "totalChars": 71798, }, "design-minimal": { "sections": [ @@ -162,10 +168,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "maybe-deck-framework", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 89314, + "totalChars": 90857, }, "design-no-ds-multitarget": { "sections": [ @@ -177,10 +184,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 65686, + "totalChars": 67229, }, "example-prompt": { "sections": [ @@ -192,10 +200,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 63933, + "totalChars": 65476, }, "freeform-other": { "sections": [ @@ -208,10 +217,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "maybe-deck-framework", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 93010, + "totalChars": 94553, }, "freeform-other-no-deck-signal": { "sections": [ @@ -222,10 +232,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 62856, + "totalChars": 64399, }, "media-image": { "sections": [ @@ -234,10 +245,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-generation-contract", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 48154, + "totalChars": 49697, }, "memory-hooks-off": { "sections": [ @@ -251,10 +263,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "maybe-deck-framework", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 91542, + "totalChars": 93085, }, "plan-mode": { "sections": [ @@ -266,10 +279,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 66880, + "totalChars": 68423, }, "plugin-stages": { "sections": [ @@ -283,10 +297,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "maybe-deck-framework", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 89410, + "totalChars": 90953, }, "skip-discovery-brief": { "sections": [ @@ -298,10 +313,11 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "project-metadata", "media-dispatch-hint", "filesystem-handoff-override", + "skill-write-boundary", "clarifying-questions", "role-marker-guard", ], - "totalChars": 63444, + "totalChars": 64987, }, "slim-design-full-stack": { "sections": [ @@ -326,9 +342,10 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "active-skill", "project-metadata", "media-dispatch-hint", + "skill-write-boundary", "role-marker-guard", ], - "totalChars": 47995, + "totalChars": 49538, }, "slim-freeform-no-deck-signal": { "sections": [ @@ -336,9 +353,10 @@ exports[`composeSystemPrompt — scenario × section golden matrix > keeps the s "slim-core-charter", "project-metadata", "media-dispatch-hint", + "skill-write-boundary", "role-marker-guard", ], - "totalChars": 38719, + "totalChars": 40262, }, } `; diff --git a/apps/daemon/tests/prompts/system-prompt-matrix.test.ts b/apps/daemon/tests/prompts/system-prompt-matrix.test.ts index e4965b42b5..b9eca2d308 100644 --- a/apps/daemon/tests/prompts/system-prompt-matrix.test.ts +++ b/apps/daemon/tests/prompts/system-prompt-matrix.test.ts @@ -71,6 +71,7 @@ const SECTION_MARKERS = [ ['critique-panel', '## Panelist role definitions'], ['active-ds-visual-direction-override', '## Active design system visual direction'], ['filesystem-handoff-override', '## Filesystem handoff'], + ['skill-write-boundary', '## Editing skills'], ['clarifying-questions', '## Structured clarification on any turn'], ['role-marker-guard', ROLE_MARKER_GUARD_SENTINEL], ] as const satisfies ReadonlyArray; diff --git a/apps/daemon/tests/skill-write-guidance.test.ts b/apps/daemon/tests/skill-write-guidance.test.ts new file mode 100644 index 0000000000..00748b320e --- /dev/null +++ b/apps/daemon/tests/skill-write-guidance.test.ts @@ -0,0 +1,212 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { composeSystemPrompt } from '../src/prompts/system.js'; + +// Regression cover for the "skills are read-only and the agent was never told" +// report (0.21.0, packaged stable). A user asked the agent to update one of his +// own skills under the daemon data dir. Every write came back `Operation not +// permitted` — by design: `resolveChatExtraAllowedDirs` deliberately keeps skill +// roots out of the agent's writable allowlist, and Codex gets no skill dir at +// all (PR #622: "Do not add read-only resource/reference directories to Codex +// --add-dir"). +// +// Nothing in the composed system prompt said so. With no stated boundary and no +// stated exit, the agent invented one: it told the user to open Open Design's +// settings and add the directory to a "文件系统 / 工作区 / 可写目录 / writable +// roots" option. No such setting exists — not for writable roots, not for a +// sandbox mode, not for an approval policy. The user hunted for it, could not +// find it, and filed a diagnostics bundle. +// +// These tests pin the three things the prompt must state so the failure mode +// cannot come back: +// 1. the boundary — skill directories are not writable through file tools; +// 2. the staged copy — `.od-skills//` is the one skill path the +// permission error does NOT cover. `withSkillRootPreamble` advertises it +// as the primary **Skill root** and it lives inside the agent's writable +// project cwd, so a write there succeeds, looks like an install, and is +// wiped when `stageActiveSkill` re-stages next turn — the same +// misdirection as the invented setting, without the error to stop it; +// 3. the exit — where a human actually edits a skill in the UI; +// 4. the prohibition on inventing product settings, worded so it stays a +// statement about today rather than a promise we never make one. +// +// The assertions match on meaning rather than importing the implementation +// constant: a same-file copy would go green against any wording, including a +// wording that reintroduces the bug. +// +// Every assertion runs against the extracted section, never the whole prompt. +// Scoping matters: an earlier draft asserted `/today/i` over the full document +// and passed on `main` with no implementation at all, because some unrelated +// section happened to contain the word. A guard that cannot go red is not a +// guard. +const SECTION_HEADING = '## Editing skills'; + +function skillGuidanceSection(prompt: string): string { + const start = prompt.indexOf(SECTION_HEADING); + if (start < 0) return ''; + const rest = prompt.slice(start + SECTION_HEADING.length); + const end = rest.search(/\n(?:#{1,3} |---)/u); + return end < 0 ? rest : rest.slice(0, end); +} + +function composedSkillGuidance(): string { + return skillGuidanceSection(composeSystemPrompt({ metadata: { kind: 'prototype' } })); +} + +describe('composeSystemPrompt — skill write guidance', () => { + it('emits a dedicated skill-editing section', () => { + expect(composedSkillGuidance()).not.toBe(''); + }); + + it('states that skill directories are not writable through file tools', () => { + const section = composedSkillGuidance(); + + // The boundary itself, and the observable symptom the agent will hit. + expect(section).toMatch( + /skill (directories|folders|roots)[^.]*(are not|is not|aren't|cannot be)[^.]*writ/i, + ); + expect(section).toMatch(/operation not permitted|permission denied|will fail/i); + }); + + it('names the staged .od-skills copy as a write that does not persist', () => { + const section = composedSkillGuidance(); + + // `/operation not permitted/` above cannot go red on this path: the staged + // copy is inside the writable project cwd, so the write succeeds. The + // section has to say so explicitly, or "write it into the project folder" + // reads as an invitation to install into `.od-skills//SKILL.md`. + expect(section).toMatch(/\.od-skills/); + expect(section).toMatch(/(appear to succeed|succeed[^.]*(but|while)|does not (update|persist))/i); + expect(section).toMatch(/(do not|don't|never) edit that path/i); + }); + + it('keeps the handover file out of the staged copy', () => { + // The proposal is a new file in the project folder, explicitly not under + // the staged skill root. + expect(composedSkillGuidance()).toMatch(/never under `\.od-skills\/`/i); + }); + + it('points at the real UI location where a skill is edited', () => { + // The actual entry point is the Integration view's Skills tab + // (`IntegrationsView.tsx`, `integrations.tabLabel.skills`) — NOT a + // "Settings > Skills" page, which does not exist. Naming the wrong + // surface here would reproduce the original bug in our own voice. + expect(composedSkillGuidance()).toMatch(/Integration[^.]{0,40}Skills/i); + }); + + it('forbids inventing sandbox / writable-root / approval-policy settings', () => { + const section = composedSkillGuidance(); + + // Hyphen-tolerant: the prose reads "writable-directory" / "approval-policy" + // in attributive position and "writable roots" when naming the setting. + expect(section).toMatch(/writable[- ](root|director)/i); + expect(section).toMatch(/approval[- ]polic/i); + expect(section).toMatch( + /(do not|don't|never)[^.]*(invent|fabricate|make up|claim|tell the user to)/i, + ); + }); + + it('words the missing settings as "not today" rather than "never"', () => { + const section = composedSkillGuidance(); + + // Product asked for this explicitly: contributors read an absolute "we do + // not have this" as "this is rejected". Keep it a statement about the + // current build so a future writable-roots surface is not pre-refused. + expect(section).toMatch(/do(es)? not currently|not yet|current build/i); + expect(section).not.toMatch( + /(will never|never going to|has no plans|not planned|we do not (support|intend))/i, + ); + }); +}); + +// The same boundary, on the OTHER prompt path. +// +// `server.ts` picks exactly one composer per run: +// +// const prompt = odNextStrategyRecipe +// ? composeOdNextStrategyCorePromptV2(odNextStrategyRecipe) +// : composeSystemPrompt(systemPromptInputs); +// +// so everything the suite above pins is absent whenever the OD Next strategy +// is assigned — `composeSystemPrompt` is not called at all, and there is no +// third always-resident block to fall back on. OD Next is not a path where +// this is hypothetical: `materializeFrozenSkillPackage` writes the frozen +// Skill package into `/.od-skills/`, and the strategy prompt hands the +// agent those exact roots (`materializedRoot` in the Skill roster, +// `Frozen side-file root:` in each Skill body). Writable directory, +// advertised path, no stated boundary — the original report's setup, with a +// frozen-identity break on top. +// +// The OD Next core prompt is a bundled plugin asset rather than a TypeScript +// constant, so this reads the shipped file. Content identity is computed from +// these bytes at apply time (`buildStrategyPackageIdentity`), which is why the +// manifest's declared `assets.core.version` moves with the text. +const OD_NEXT_CORE_PROMPT = path.resolve( + import.meta.dirname, + '../../../plugins/_official/scenarios/od-next-strategy/assets/core-system-prompt.md', +); + +function odNextBoundarySection(): string { + const text = readFileSync(OD_NEXT_CORE_PROMPT, 'utf8'); + const heading = '## Agent and runtime boundaries'; + const start = text.indexOf(heading); + if (start < 0) return ''; + const rest = text.slice(start + heading.length); + const end = rest.search(/\n## /u); + const section = end < 0 ? rest : rest.slice(0, end); + // This asset is hard-wrapped prose, so a sentence the assertions below look + // for is routinely split across two lines. Collapse runs of whitespace so a + // reflow (which changes no meaning) cannot turn a guard red, while a deleted + // or reworded sentence still does. + return section.replaceAll(/\s+/gu, ' '); +} + +describe('OD Next core strategy prompt — skill write guidance', () => { + // Locator sanity only: the heading predates this change, so this one stays + // green on `main`. The guards that can actually go red are below it. + it('locates the section the guidance belongs to', () => { + expect(odNextBoundarySection()).not.toBe(''); + }); + + it('states that skill directories are not writable through file tools', () => { + const section = odNextBoundarySection(); + + expect(section).toMatch( + /skill (directories|folders|roots)[^.]*(are not|is not|aren't|cannot be)[^.]*writ/i, + ); + expect(section).toMatch(/operation not permitted|permission denied|will fail/i); + }); + + it('names the materialized .od-skills roots as writes that do not persist', () => { + const section = odNextBoundarySection(); + + // Scoped to this section, not the whole file: `.od-skills` appears in the + // task-profile assets too, so a document-wide match could not go red. + expect(section).toMatch(/\.od-skills/); + expect(section).toMatch(/appear to succeed while updating nothing/i); + expect(section).toMatch(/never write them|do not write them/i); + }); + + it('keeps the handover file out of the materialized copy', () => { + expect(odNextBoundarySection()).toMatch(/never under `\.od-skills\/`/i); + }); + + it('points at the real UI location where a skill is edited', () => { + expect(odNextBoundarySection()).toMatch(/Integration[^.]{0,40}Skills/i); + }); + + it('forbids inventing sandbox / writable-root / approval-policy settings', () => { + const section = odNextBoundarySection(); + + expect(section).toMatch(/writable[- ](root|director)/i); + expect(section).toMatch(/approval[- ]polic/i); + expect(section).toMatch( + /(do not|don't|never)[^.]*(invent|fabricate|make up|claim|tell the user to)/i, + ); + expect(section).toMatch(/do(es)? not currently|not yet|current build/i); + expect(section).not.toMatch( + /(will never|never going to|has no plans|not planned|we do not (support|intend))/i, + ); + }); +}); diff --git a/plugins/_official/scenarios/od-next-strategy/assets/core-system-prompt.md b/plugins/_official/scenarios/od-next-strategy/assets/core-system-prompt.md index c857d091af..9f3b3aaa31 100644 --- a/plugins/_official/scenarios/od-next-strategy/assets/core-system-prompt.md +++ b/plugins/_official/scenarios/od-next-strategy/assets/core-system-prompt.md @@ -159,6 +159,27 @@ formats follow the general orchestration Skill. - Open Design owns session creation, continuation, and expiry. Handle only the current request and the continuation instructions you receive; never manage sessions yourself. +- Skill directories are not writable through your file tools. A skill is part + of your own instructions, so it is never edited as a side effect of a task; + a write attempt returns `Operation not permitted`, and retrying, changing + the path, or routing the same write through a shell command will not help. +- The `.od-skills/` roots this strategy names — every `materializedRoot` in + the Skill roster and every `Frozen side-file root` in a Skill body — are + read-only materialized copies inside the project. They sit in a directory + you can write, so an edit there will appear to succeed while updating + nothing, and it breaks the frozen Skill identity Open Design verifies. Read + them; never write them, and never report a successful write there as having + created or updated a Skill. +- When the user asks you to create or change a Skill, do the work and hand it + over instead of installing it yourself: write the proposal as a new `.md` + file in the project folder — never under `.od-skills/` — and tell the user + to paste it in through the Integration view's Skills tab, which is where + Skills are edited in the app. +- Open Design does not currently expose a sandbox mode, a writable-directory + or "writable roots" list, or an approval-policy setting. Do not tell the + user to look for one, and do not invent a settings path, menu, or option + name to explain the failure — they will go looking and find nothing. State + the limitation plainly and point at the Skills tab instead. ## Design baseline diff --git a/plugins/_official/scenarios/od-next-strategy/open-design.json b/plugins/_official/scenarios/od-next-strategy/open-design.json index 9db18736cd..e15a76ef9d 100644 --- a/plugins/_official/scenarios/od-next-strategy/open-design.json +++ b/plugins/_official/scenarios/od-next-strategy/open-design.json @@ -37,7 +37,7 @@ "assets": { "core": { "path": "./assets/core-system-prompt.md", - "version": "2.1.0" + "version": "2.2.0" }, "orchestration": { "path": "./assets/general-orchestration.md",