diff --git a/.changeset/propose-includes-specs.md b/.changeset/propose-includes-specs.md new file mode 100644 index 0000000000..137c69fc98 --- /dev/null +++ b/.changeset/propose-includes-specs.md @@ -0,0 +1,11 @@ +--- +"@fission-ai/openspec": patch +--- + +### Fixed + +- **`/opsx:propose` and `/opsx:ff` no longer finish a change with no spec written.** The workflows listed only `proposal`/`design`/`tasks` and treated the apply phase's `tasks` artifact as the stop condition — but `status` marks an artifact `done` as soon as a matching file exists, so writing `tasks.md` early satisfied the loop while `specs//spec.md` was never created (a spec-less change in a spec-driven tool). The loop now derives the full required set — every apply dependency plus everything it transitively `requires` — from a single `status` call, creates each missing artifact, and only skips one when its own `instruction` field marks it conditional. (#1260, #788) + +### Changed + +- **`openspec status --json` now reports each artifact's `requires` edges.** Every entry in the `artifacts` array carries a `requires` array of the ids it directly depends on, present for every status (including `done`) so agents can compute the transitive required set from `status` alone. Additive and backward-compatible — existing fields are unchanged. diff --git a/docs/agent-contract.md b/docs/agent-contract.md index 1aa5f55530..28ae66a26c 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -55,7 +55,7 @@ Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id `{ "items": [ { "id", "type": "change"|"spec", "valid", "issues": [ { "level", "path", "message", "line"?, "column"? } ], "durationMs" } ], "summary": { "totals": {items,passed,failed}, "byType": {...} }, "version": "1.0", "root" }`. Exit 1 when any item fails. ### 4.4 `status --json` -`{ "changeName", "schemaName", "planningHome"?: { "kind", "root", "changesDir", "defaultSchema" }, "changeRoot", "artifactPaths": { "": {outputPath, resolvedOutputPath, existingOutputPaths} }, "nextSteps": ["..."], "actionContext": { "mode": "repo-local", "sourceOfTruth": "repo", "planningArtifacts", "linkedContext", "allowedEditRoots", "requiresAffectedAreaSelection", "constraints" }, "isComplete", "applyRequires", "artifacts": [ {id, outputPath, status: "done"|"ready"|"blocked", missingDeps?} ], "root" }`. No active changes: `{ "changes": [], "message", "root" }`, exit 0. +`{ "changeName", "schemaName", "planningHome"?: { "kind", "root", "changesDir", "defaultSchema" }, "changeRoot", "artifactPaths": { "": {outputPath, resolvedOutputPath, existingOutputPaths} }, "nextSteps": ["..."], "actionContext": { "mode": "repo-local", "sourceOfTruth": "repo", "planningArtifacts", "linkedContext", "allowedEditRoots", "requiresAffectedAreaSelection", "constraints" }, "isComplete", "applyRequires", "artifacts": [ {id, outputPath, status: "done"|"ready"|"blocked", requires, missingDeps?} ], "root" }`. Each artifact's `requires` is its direct dependency ids (present for every status, so the transitive required set is computable even when the artifact is `done`); `missingDeps` appears only when `blocked`. No active changes: `{ "changes": [], "message", "root" }`, exit 0. ### 4.5 `instructions --json` `{ "changeName", "artifactId", "schemaName", "changeDir", "planningHome"?, "outputPath", "resolvedOutputPath", "existingOutputPaths", "description", "instruction"?, "context"?, "rules"?, "references"?: ReferenceIndexEntry[], "template", "dependencies": [{id,done,path,description}], "unlocks", "root" }`. diff --git a/docs/cli.md b/docs/cli.md index 9988131430..8a75845cb2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -702,10 +702,10 @@ Progress: 2/4 artifacts complete "isComplete": false, "applyRequires": ["tasks"], "artifacts": [ - {"id": "proposal", "outputPath": "proposal.md", "status": "done"}, - {"id": "design", "outputPath": "design.md", "status": "ready"}, - {"id": "specs", "outputPath": "specs/**/*.md", "status": "done"}, - {"id": "tasks", "outputPath": "tasks.md", "status": "blocked", "missingDeps": ["design"]} + {"id": "proposal", "outputPath": "proposal.md", "status": "done", "requires": []}, + {"id": "design", "outputPath": "design.md", "status": "ready", "requires": ["proposal"]}, + {"id": "specs", "outputPath": "specs/**/*.md", "status": "done", "requires": ["proposal"]}, + {"id": "tasks", "outputPath": "tasks.md", "status": "blocked", "requires": ["specs", "design"], "missingDeps": ["design"]} ] } ``` diff --git a/openspec/specs/cli-artifact-workflow/spec.md b/openspec/specs/cli-artifact-workflow/spec.md index 0d9144e8c5..6315db96c8 100644 --- a/openspec/specs/cli-artifact-workflow/spec.md +++ b/openspec/specs/cli-artifact-workflow/spec.md @@ -34,6 +34,12 @@ The system SHALL display artifact completion status for a change, including scaf - `changeName`, `schemaName`, `isComplete`, `artifacts` array - `applyRequires`: array of artifact IDs needed for apply phase +#### Scenario: Status JSON exposes each artifact's dependency edges + +- **WHEN** user runs `openspec status --change --json` +- **THEN** every entry in the `artifacts` array includes `requires`: the array of artifact IDs it directly depends on +- **AND** `requires` is present regardless of the artifact's status, so a `done` artifact still reports its dependencies (letting agents compute the transitive required set from status alone) + #### Scenario: Status on scaffolded change - **WHEN** user runs `openspec status --change ` on a change with no artifacts diff --git a/skills/openspec-ff-change/SKILL.md b/skills/openspec-ff-change/SKILL.md index a27bc5ab39..70bf2960fb 100644 --- a/skills/openspec-ff-change/SKILL.md +++ b/skills/openspec-ff-change/SKILL.md @@ -38,10 +38,10 @@ Fast-forward through artifact creation - generate everything needed to start imp ``` Parse the JSON to get: - `applyRequires`: array of artifact IDs needed before implementation (e.g., `["tasks"]`) - - `artifacts`: list of all artifacts with their status and dependencies + - `artifacts`: list of all artifacts, each with its `status` and its `requires` edges (the artifact IDs it directly depends on) - `planningHome`, `changeRoot`, `artifactPaths`, and `actionContext`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -61,14 +61,18 @@ Fast-forward through artifact creation - generate everything needed to start imp - `dependencies`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the `instruction` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at `resolvedOutputPath` - - Otherwise create the artifact file using `template` as the structure and write it to `resolvedOutputPath` + - Otherwise create the artifact file using `template` as the structure and write it to `resolvedOutputPath`. If `resolvedOutputPath` is a glob, follow `instruction` to choose the concrete file path - Apply `context` and `rules` as constraints - but do NOT copy them into the file - Show brief progress: "✓ Created " - b. **Continue until all `applyRequires` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just `apply.requires`)** - After creating each artifact, re-run `openspec status --change "" --json` - - Check if every artifact ID in `applyRequires` has `status: "done"` in the artifacts array - - Stop when all `applyRequires` artifacts are done + - The required set is `applyRequires` plus every artifact reachable from those by following the `requires` edges in `status --json` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - `status` is file-existence only, so an `applyRequires` artifact reading `done` does NOT mean its dependencies exist - writing `tasks.md` early marks `tasks` done while `specs` was never written. Use each artifact's `requires` edges, not its `status`, to build the required set: a `done` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own `instruction` says it is conditional: run `openspec instructions --change "" --json` and skip only if its `instruction` field marks it optional (e.g. "create only if..."). Spec-driven's `design.md` qualifies; `specs` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still `blocked` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is `done` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -83,8 +87,8 @@ Fast-forward through artifact creation - generate everything needed to start imp After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run `/openspec-apply-change` or ask me to implement to start working on the tasks." **Artifact Creation Guidelines** @@ -99,7 +103,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's `apply.requires`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in `apply.requires` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, suggest continuing that change instead diff --git a/skills/openspec-propose/SKILL.md b/skills/openspec-propose/SKILL.md index 76446528fd..07493a840e 100644 --- a/skills/openspec-propose/SKILL.md +++ b/skills/openspec-propose/SKILL.md @@ -11,8 +11,9 @@ metadata: Propose a new change - create the change and generate all artifacts in one step. -I'll create a change with artifacts: +I'll create a change with the artifacts your schema defines. With the default spec-driven schema that is: - proposal.md (what & why) +- `specs//spec.md` (what the system must do - a delta, not the main spec) - design.md (how) - tasks.md (implementation steps) @@ -47,10 +48,10 @@ When ready to implement, run /openspec-apply-change ``` Parse the JSON to get: - `applyRequires`: array of artifact IDs needed before implementation (e.g., `["tasks"]`) - - `artifacts`: list of all artifacts with their status and dependencies + - `artifacts`: list of all artifacts, each with its `status` and its `requires` edges (the artifact IDs it directly depends on) - `planningHome`, `changeRoot`, `artifactPaths`, and `actionContext`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -70,14 +71,18 @@ When ready to implement, run /openspec-apply-change - `dependencies`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the `instruction` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at `resolvedOutputPath` - - Otherwise create the artifact file using `template` as the structure and write it to `resolvedOutputPath` + - Otherwise create the artifact file using `template` as the structure and write it to `resolvedOutputPath`. If `resolvedOutputPath` is a glob, follow `instruction` to choose the concrete file path - Apply `context` and `rules` as constraints - but do NOT copy them into the file - Show brief progress: "Created " - b. **Continue until all `applyRequires` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just `apply.requires`)** - After creating each artifact, re-run `openspec status --change "" --json` - - Check if every artifact ID in `applyRequires` has `status: "done"` in the artifacts array - - Stop when all `applyRequires` artifacts are done + - The required set is `applyRequires` plus every artifact reachable from those by following the `requires` edges in `status --json` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - `status` is file-existence only, so an `applyRequires` artifact reading `done` does NOT mean its dependencies exist - writing `tasks.md` early marks `tasks` done while `specs` was never written. Use each artifact's `requires` edges, not its `status`, to build the required set: a `done` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own `instruction` says it is conditional: run `openspec instructions --change "" --json` and skip only if its `instruction` field marks it optional (e.g. "create only if..."). Spec-driven's `design.md` qualifies; `specs` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still `blocked` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is `done` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -92,8 +97,8 @@ When ready to implement, run /openspec-apply-change After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run `/openspec-apply-change` or ask me to implement to start working on the tasks." **Artifact Creation Guidelines** @@ -108,7 +113,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's `apply.requires`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in `apply.requires` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one diff --git a/src/core/artifact-graph/instruction-loader.ts b/src/core/artifact-graph/instruction-loader.ts index b94fa2c56d..aec7f47f22 100644 --- a/src/core/artifact-graph/instruction-loader.ts +++ b/src/core/artifact-graph/instruction-loader.ts @@ -124,6 +124,11 @@ export interface ArtifactStatus { outputPath: string; /** Status: done, ready, or blocked */ status: 'done' | 'ready' | 'blocked'; + /** Artifact IDs this artifact directly requires (its `requires` edges). + * Present for every status so callers can compute the transitive required + * set even when the artifact is already `done` (file-existence status does + * not imply its dependencies exist). */ + requires: string[]; /** Missing dependencies (only for blocked) */ missingDeps?: string[]; } @@ -406,6 +411,7 @@ export function formatChangeStatus( id: artifact.id, outputPath: artifact.generates, status: 'done' as const, + requires: artifact.requires, }; } @@ -414,6 +420,7 @@ export function formatChangeStatus( id: artifact.id, outputPath: artifact.generates, status: 'ready' as const, + requires: artifact.requires, }; } @@ -421,6 +428,7 @@ export function formatChangeStatus( id: artifact.id, outputPath: artifact.generates, status: 'blocked' as const, + requires: artifact.requires, missingDeps: blocked[artifact.id] ?? [], }; }); diff --git a/src/core/templates/workflows/ff-change.ts b/src/core/templates/workflows/ff-change.ts index b4e85e3ea7..b60d2c7b2b 100644 --- a/src/core/templates/workflows/ff-change.ts +++ b/src/core/templates/workflows/ff-change.ts @@ -40,10 +40,10 @@ ${STORE_SELECTION_GUIDANCE} \`\`\` Parse the JSON to get: - \`applyRequires\`: array of artifact IDs needed before implementation (e.g., \`["tasks"]\`) - - \`artifacts\`: list of all artifacts with their status and dependencies + - \`artifacts\`: list of all artifacts, each with its \`status\` and its \`requires\` edges (the artifact IDs it directly depends on) - \`planningHome\`, \`changeRoot\`, \`artifactPaths\`, and \`actionContext\`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -63,14 +63,18 @@ ${STORE_SELECTION_GUIDANCE} - \`dependencies\`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the \`instruction\` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at \`resolvedOutputPath\` - - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\` + - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\`. If \`resolvedOutputPath\` is a glob, follow \`instruction\` to choose the concrete file path - Apply \`context\` and \`rules\` as constraints - but do NOT copy them into the file - Show brief progress: "✓ Created " - b. **Continue until all \`applyRequires\` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just \`apply.requires\`)** - After creating each artifact, re-run \`openspec status --change "" --json\` - - Check if every artifact ID in \`applyRequires\` has \`status: "done"\` in the artifacts array - - Stop when all \`applyRequires\` artifacts are done + - The required set is \`applyRequires\` plus every artifact reachable from those by following the \`requires\` edges in \`status --json\` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - \`status\` is file-existence only, so an \`applyRequires\` artifact reading \`done\` does NOT mean its dependencies exist - writing \`tasks.md\` early marks \`tasks\` done while \`specs\` was never written. Use each artifact's \`requires\` edges, not its \`status\`, to build the required set: a \`done\` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own \`instruction\` says it is conditional: run \`openspec instructions --change "" --json\` and skip only if its \`instruction\` field marks it optional (e.g. "create only if..."). Spec-driven's \`design.md\` qualifies; \`specs\` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still \`blocked\` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is \`done\` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -85,8 +89,8 @@ ${STORE_SELECTION_GUIDANCE} After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run \`/opsx:apply\` or ask me to implement to start working on the tasks." **Artifact Creation Guidelines** @@ -101,7 +105,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in \`apply.requires\` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, suggest continuing that change instead @@ -147,10 +151,10 @@ ${STORE_SELECTION_GUIDANCE} \`\`\` Parse the JSON to get: - \`applyRequires\`: array of artifact IDs needed before implementation (e.g., \`["tasks"]\`) - - \`artifacts\`: list of all artifacts with their status and dependencies + - \`artifacts\`: list of all artifacts, each with its \`status\` and its \`requires\` edges (the artifact IDs it directly depends on) - \`planningHome\`, \`changeRoot\`, \`artifactPaths\`, and \`actionContext\`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -170,14 +174,18 @@ ${STORE_SELECTION_GUIDANCE} - \`dependencies\`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the \`instruction\` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at \`resolvedOutputPath\` - - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\` + - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\`. If \`resolvedOutputPath\` is a glob, follow \`instruction\` to choose the concrete file path - Apply \`context\` and \`rules\` as constraints - but do NOT copy them into the file - Show brief progress: "✓ Created " - b. **Continue until all \`applyRequires\` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just \`apply.requires\`)** - After creating each artifact, re-run \`openspec status --change "" --json\` - - Check if every artifact ID in \`applyRequires\` has \`status: "done"\` in the artifacts array - - Stop when all \`applyRequires\` artifacts are done + - The required set is \`applyRequires\` plus every artifact reachable from those by following the \`requires\` edges in \`status --json\` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - \`status\` is file-existence only, so an \`applyRequires\` artifact reading \`done\` does NOT mean its dependencies exist - writing \`tasks.md\` early marks \`tasks\` done while \`specs\` was never written. Use each artifact's \`requires\` edges, not its \`status\`, to build the required set: a \`done\` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own \`instruction\` says it is conditional: run \`openspec instructions --change "" --json\` and skip only if its \`instruction\` field marks it optional (e.g. "create only if..."). Spec-driven's \`design.md\` qualifies; \`specs\` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still \`blocked\` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is \`done\` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -192,8 +200,8 @@ ${STORE_SELECTION_GUIDANCE} After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run \`/opsx:apply\` to start implementing." **Artifact Creation Guidelines** @@ -208,7 +216,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in \`apply.requires\` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one diff --git a/src/core/templates/workflows/propose.ts b/src/core/templates/workflows/propose.ts index edcf2c582d..9083a2cd09 100644 --- a/src/core/templates/workflows/propose.ts +++ b/src/core/templates/workflows/propose.ts @@ -13,8 +13,9 @@ export function getOpsxProposeSkillTemplate(): SkillTemplate { description: 'Propose a new change with all artifacts generated in one step. Use when the user wants to quickly describe what they want to build and get a complete proposal with design, specs, and tasks ready for implementation.', instructions: `Propose a new change - create the change and generate all artifacts in one step. -I'll create a change with artifacts: +I'll create a change with the artifacts your schema defines. With the default spec-driven schema that is: - proposal.md (what & why) +- \`specs//spec.md\` (what the system must do - a delta, not the main spec) - design.md (how) - tasks.md (implementation steps) @@ -49,10 +50,10 @@ ${STORE_SELECTION_GUIDANCE} \`\`\` Parse the JSON to get: - \`applyRequires\`: array of artifact IDs needed before implementation (e.g., \`["tasks"]\`) - - \`artifacts\`: list of all artifacts with their status and dependencies + - \`artifacts\`: list of all artifacts, each with its \`status\` and its \`requires\` edges (the artifact IDs it directly depends on) - \`planningHome\`, \`changeRoot\`, \`artifactPaths\`, and \`actionContext\`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -72,14 +73,18 @@ ${STORE_SELECTION_GUIDANCE} - \`dependencies\`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the \`instruction\` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at \`resolvedOutputPath\` - - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\` + - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\`. If \`resolvedOutputPath\` is a glob, follow \`instruction\` to choose the concrete file path - Apply \`context\` and \`rules\` as constraints - but do NOT copy them into the file - Show brief progress: "Created " - b. **Continue until all \`applyRequires\` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just \`apply.requires\`)** - After creating each artifact, re-run \`openspec status --change "" --json\` - - Check if every artifact ID in \`applyRequires\` has \`status: "done"\` in the artifacts array - - Stop when all \`applyRequires\` artifacts are done + - The required set is \`applyRequires\` plus every artifact reachable from those by following the \`requires\` edges in \`status --json\` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - \`status\` is file-existence only, so an \`applyRequires\` artifact reading \`done\` does NOT mean its dependencies exist - writing \`tasks.md\` early marks \`tasks\` done while \`specs\` was never written. Use each artifact's \`requires\` edges, not its \`status\`, to build the required set: a \`done\` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own \`instruction\` says it is conditional: run \`openspec instructions --change "" --json\` and skip only if its \`instruction\` field marks it optional (e.g. "create only if..."). Spec-driven's \`design.md\` qualifies; \`specs\` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still \`blocked\` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is \`done\` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -94,8 +99,8 @@ ${STORE_SELECTION_GUIDANCE} After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run \`/opsx:apply\` or ask me to implement to start working on the tasks." **Artifact Creation Guidelines** @@ -110,7 +115,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in \`apply.requires\` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one @@ -129,8 +134,9 @@ export function getOpsxProposeCommandTemplate(): CommandTemplate { tags: ['workflow', 'artifacts', 'experimental'], content: `Propose a new change - create the change and generate all artifacts in one step. -I'll create a change with artifacts: +I'll create a change with the artifacts your schema defines. With the default spec-driven schema that is: - proposal.md (what & why) +- \`specs//spec.md\` (what the system must do - a delta, not the main spec) - design.md (how) - tasks.md (implementation steps) @@ -165,10 +171,10 @@ ${STORE_SELECTION_GUIDANCE} \`\`\` Parse the JSON to get: - \`applyRequires\`: array of artifact IDs needed before implementation (e.g., \`["tasks"]\`) - - \`artifacts\`: list of all artifacts with their status and dependencies + - \`artifacts\`: list of all artifacts, each with its \`status\` and its \`requires\` edges (the artifact IDs it directly depends on) - \`planningHome\`, \`changeRoot\`, \`artifactPaths\`, and \`actionContext\`: path and scope context. Use these instead of assuming repo-local paths. -4. **Create artifacts in sequence until apply-ready** +4. **Create every artifact in the required set** Use a todo list to track progress through the artifacts. @@ -188,14 +194,18 @@ ${STORE_SELECTION_GUIDANCE} - \`dependencies\`: Completed artifacts to read for context - Read any completed dependency files for context - always re-read them from disk, even if you saw them earlier in the conversation (the user may have edited them) - If the \`instruction\` field delegates creation to a specific skill or command, invoke it to produce the artifact instead of writing the file yourself, then verify the artifact file exists at \`resolvedOutputPath\` - - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\` + - Otherwise create the artifact file using \`template\` as the structure and write it to \`resolvedOutputPath\`. If \`resolvedOutputPath\` is a glob, follow \`instruction\` to choose the concrete file path - Apply \`context\` and \`rules\` as constraints - but do NOT copy them into the file - Show brief progress: "Created " - b. **Continue until all \`applyRequires\` artifacts are complete** + b. **Continue until every artifact in the required set exists (not just \`apply.requires\`)** - After creating each artifact, re-run \`openspec status --change "" --json\` - - Check if every artifact ID in \`applyRequires\` has \`status: "done"\` in the artifacts array - - Stop when all \`applyRequires\` artifacts are done + - The required set is \`applyRequires\` plus every artifact reachable from those by following the \`requires\` edges in \`status --json\` - walk them transitively (spec-driven closes over proposal, specs, design, tasks). Leave artifacts outside that set alone + - \`status\` is file-existence only, so an \`applyRequires\` artifact reading \`done\` does NOT mean its dependencies exist - writing \`tasks.md\` early marks \`tasks\` done while \`specs\` was never written. Use each artifact's \`requires\` edges, not its \`status\`, to build the required set: a \`done\` artifact still lists what it depends on + - Create every artifact in the required set that is missing, then re-check - creating one can unblock others + - Skip one only when its own \`instruction\` says it is conditional: run \`openspec instructions --change "" --json\` and skip only if its \`instruction\` field marks it optional (e.g. "create only if..."). Spec-driven's \`design.md\` qualifies; \`specs\` never does. Tell the user, and do not reconsider it + - Dependencies are enablers, not gates: if a required artifact is still \`blocked\` only because you skipped a conditional dependency, write it anyway + - Stop when every artifact in the required set is \`done\` or was deliberately skipped c. **If an artifact requires user input** (unclear context): - Use **AskUserQuestion tool** to clarify @@ -210,8 +220,8 @@ ${STORE_SELECTION_GUIDANCE} After completing all artifacts, summarize: - Change name and location -- List of artifacts created with brief descriptions -- What's ready: "All artifacts created! Ready for implementation." +- List of artifacts created with brief descriptions, plus any conditional artifact you skipped and why +- What's ready: "All artifacts needed for implementation are ready." - Prompt: "Run \`/opsx:apply\` to start implementing." **Artifact Creation Guidelines** @@ -226,7 +236,7 @@ After completing all artifacts, summarize: - These guide what you write, but should never appear in the output **Guardrails** -- Create ALL artifacts needed for implementation (as defined by schema's \`apply.requires\`) +- Create every artifact the apply phase transitively depends on, not just the ids listed in \`apply.requires\` - Always read dependency artifacts before creating a new one - re-read from disk, not from conversation memory (files may have changed since you last saw them) - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, ask if user wants to continue it or create a new one diff --git a/test/commands/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index 9becfff376..90f80edc62 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -544,6 +544,7 @@ apply: id: 'specs', outputPath: 'specs/*/spec.md', status: 'done', + requires: [], }, ]); diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index 9d8f612cd8..12ba6d9c99 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -592,6 +592,30 @@ rules: expect(tasks?.missingDeps).toContain('design'); }); + it('should expose each artifact\'s requires edges regardless of status', () => { + const changeDir = path.join(tempDir, 'openspec', 'changes', 'my-change'); + fs.mkdirSync(changeDir, { recursive: true }); + // Prewritten-tasks scenario: only tasks.md exists. `tasks` reads `done` + // by file existence, but its specs/design dependencies were never written. + fs.writeFileSync(path.join(changeDir, 'tasks.md'), '# Tasks'); + + const context = loadChangeContext(tempDir, 'my-change'); + const status = formatChangeStatus(context); + + // A done artifact must still carry its requires edges so callers can + // compute the transitive required set (alfred's PR #1412 blocker). + const tasks = status.artifacts.find(a => a.id === 'tasks'); + expect(tasks?.status).toBe('done'); + expect(tasks?.requires).toEqual(expect.arrayContaining(['specs', 'design'])); + + // proposal has no dependencies -> empty edges, not undefined. + const proposal = status.artifacts.find(a => a.id === 'proposal'); + expect(proposal?.requires).toEqual([]); + + // Every artifact carries the field, whatever its status. + expect(status.artifacts.every(a => Array.isArray(a.requires))).toBe(true); + }); + it('should sort artifacts in build order', () => { const context = loadChangeContext(tempDir, 'my-change'); const status = formatChangeStatus(context); diff --git a/test/core/templates/propose.test.ts b/test/core/templates/propose.test.ts new file mode 100644 index 0000000000..ce519ee3b4 --- /dev/null +++ b/test/core/templates/propose.test.ts @@ -0,0 +1,202 @@ +import path from 'path'; +import { fileURLToPath } from 'url'; +import { describe, expect, it } from 'vitest'; + +import { + getOpsxProposeSkillTemplate, + getOpsxProposeCommandTemplate, + getFfChangeSkillTemplate, + getOpsxFfCommandTemplate, +} from '../../../src/core/templates/skill-templates.js'; +import { loadSchema } from '../../../src/core/artifact-graph/schema.js'; + +const proposeBodies: Array<[string, string]> = [ + ['propose skill', getOpsxProposeSkillTemplate().instructions], + ['propose command', getOpsxProposeCommandTemplate().content], +]; + +// ff runs the byte-identical artifact loop, so it carries the identical guards. +const loopBodies: Array<[string, string]> = [ + ...proposeBodies, + ['ff skill', getFfChangeSkillTemplate().instructions], + ['ff command', getOpsxFfCommandTemplate().content], +]; + +const repoRoot = path.resolve(fileURLToPath(new URL('.', import.meta.url)), '../../..'); +const defaultSchema = loadSchema(path.join(repoRoot, 'schemas', 'spec-driven', 'schema.yaml')); + +/** The opening list that tells the agent which artifacts propose will produce. */ +function artifactPreamble(body: string): string { + const start = body.indexOf("I'll create a change with"); + const end = body.indexOf('When ready to implement'); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + return body.slice(start, end); +} + +describe('propose preamble', () => { + // #788/#1260: the preamble advertised proposal/design/tasks only, so agents + // treated specs as optional and produced changes with no spec at all. + // Derived from the schema so a new artifact cannot go unadvertised. + it('advertises every artifact the default schema defines (#788, #1260)', () => { + const ids = defaultSchema.artifacts.map(artifact => artifact.id); + expect(ids).toContain('specs'); + + for (const [label, body] of proposeBodies) { + const preamble = artifactPreamble(body); + for (const id of ids) { + expect(preamble, `${label} preamble is missing the "${id}" artifact`).toContain(id); + } + } + }); +}); + +describe('artifact loop guards (propose and ff)', () => { + // `status` is file-existence based (detectCompleted), so writing tasks.md before + // specs flips tasks to done and satisfies a bare applyRequires stop condition + // with specs never created. That is the #1260 failure chain. + it('warns that a done applyRequires artifact does not imply its deps exist (#788, #1260)', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toMatch(/file-existence only/i); + expect(body, label).toMatch(/does NOT mean its dependencies exist/i); + } + }); + + // Scoped to the applyRequires closure, not to every `ready` artifact: a custom + // schema may define artifacts outside it (e.g. a post-implementation retro) + // that propose has no business creating. + it('scopes the required set to the applyRequires dependency closure', () => { + for (const [label, body] of loopBodies) { + // Names the seed the walk starts from (`from those`) so an agent cannot + // read it as "every artifact that has requires edges" = the whole list. + expect(body, label).toContain('reachable from those by following the `requires` edges'); + // Points at status --json specifically (instructions calls the edges `dependencies`). + expect(body, label).toContain('in `status --json`'); + expect(body, label).toContain('walk them transitively'); + expect(body, label).toContain('Leave artifacts outside that set alone'); + } + }); + + // alfred's PR #1412 blocker: `status --json` must carry the `requires` edges, + // and the loop must derive the set from those edges rather than from `status`. + // A `done` artifact hides nothing about its deps if the agent reads its edges. + it('builds the required set from requires edges, not from status (#1412 review)', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + "Use each artifact's `requires` edges, not its `status`, to build the required set" + ); + expect(body, label).toContain('a `done` artifact still lists what it depends on'); + } + }); + + // The status-JSON parse list must document the `requires` field the loop relies on. + it('documents the requires edges in the status JSON it tells the agent to parse', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'each with its `status` and its `requires` edges' + ); + } + }); + + it('creates every missing artifact in the set and re-checks for cascades', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain('Create every artifact in the required set that is missing'); + expect(body, label).toMatch(/re-check - creating one can unblock others/i); + } + }); + + // specs must not be skippable — `openspec validate` rejects a change with no + // deltas. "Required" is not machine-readable (the graph has tasks requiring + // both specs and design), but the artifact's own instruction is: spec-driven's + // design says "create only if any apply", specs says nothing of the kind. + it('permits skipping only artifacts their own instruction marks conditional', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'Skip one only when its own `instruction` says it is conditional' + ); + expect(body, label).toContain('do not reconsider it'); + } + }); + + // The skip decision hinges on reading the artifact's `instruction` field, so + // the loop must explicitly tell the agent to fetch it before skipping - + // otherwise a momentum-driven agent can skip specs without ever checking. + it('makes the agent fetch and read the instruction field before skipping', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'run `openspec instructions --change "" --json` and skip only if its `instruction` field marks it optional' + ); + expect(body, label).toContain('`specs` never does'); + } + }); + + // The 4b heading must not re-state the buggy stop condition (apply.requires + // alone); it has to point the agent at the whole required set. + it('frames the loop around the required set, not apply.requires alone', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'Continue until every artifact in the required set exists (not just `apply.requires`)' + ); + expect(body, label).not.toContain( + 'Continue until every artifact the apply phase depends on exists' + ); + } + }); + + // The step-4 TITLE must not use "apply-ready" either: in the prewritten-tasks + // case the change is already apply-ready when step 4 begins, so a title of + // "create ... until apply-ready" invites the exact early-stop this PR kills. + it('titles the create step around the required set, not "apply-ready"', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain('**Create every artifact in the required set**'); + expect(body, label).not.toContain('Create artifacts in sequence until apply-ready'); + expect(body, label).not.toMatch(/^\s*4\.\s.*apply-ready/m); + } + }); + + // Without this the loop deadlocks: skipping design leaves tasks blocked + // forever, no artifact is ready, and the stop condition can never be met. + // docs/concepts.md: "Dependencies are enablers, not gates." + it('authorizes writing a blocked artifact whose only blocker was skipped', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain('Dependencies are enablers, not gates'); + expect(body, label).toMatch( + /still `blocked` only because you skipped a conditional dependency, write it anyway/ + ); + } + }); + + // The stop condition must cover the whole required set. A bare "stop when + // applyRequires is done" is the lenient rule #1260 blames. + it('stops on the whole required set, not on applyRequires alone', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'Stop when every artifact in the required set is `done` or was deliberately skipped' + ); + expect(body, label).not.toContain('Stop when all `applyRequires` artifacts are done'); + } + }); + + // The Guardrails section used to define completeness as `apply.requires`, + // which is exactly the premise this fix refutes. + it('does not define completeness as apply.requires in the guardrails', () => { + for (const [label, body] of loopBodies) { + expect(body, label).not.toMatch( + /Create ALL artifacts needed for implementation \(as defined by schema's `apply\.requires`\)/ + ); + expect(body, label).toContain( + 'Create every artifact the apply phase transitively depends on' + ); + } + }); + + // specs `generates` a glob (specs/**/*.md), so an agent told only to "write it + // to resolvedOutputPath" would create a directory literally named `**`. + it('tells the agent how to resolve a glob output path', () => { + for (const [label, body] of loopBodies) { + expect(body, label).toContain( + 'is a glob, follow `instruction` to choose the concrete file path' + ); + } + }); +}); diff --git a/test/core/templates/skill-templates-parity.test.ts b/test/core/templates/skill-templates-parity.test.ts index 1a106dfceb..36705d39cf 100644 --- a/test/core/templates/skill-templates-parity.test.ts +++ b/test/core/templates/skill-templates-parity.test.ts @@ -41,14 +41,14 @@ const EXPECTED_FUNCTION_HASHES: Record = { getNewChangeSkillTemplate: '39663a6d2037e6697020393a66f6327506e3e3bc573b7a3556dcb7f9457dc51d', getContinueChangeSkillTemplate: '912ce98855bcea351a73730c7ac18505e21512266eac8082351ef72ddfa63906', getApplyChangeSkillTemplate: '0f5a15fc7fb9ad6059a5643d0e01365d27642637a4aaebf182f9eabb45348197', - getFfChangeSkillTemplate: '25b584cdda0b99c704dbe473b0dfae084af2fac6f4ca27fe7422fb8789b0fe16', + getFfChangeSkillTemplate: '3e2cd56f2b73299fd008e08f61c778d46b098c58c11185a1f1113a92f66f259b', getSyncSpecsSkillTemplate: '32c3169e1ee0345a174c0bacb8fd16db73477cc006d8cedbedc6077233c5461b', getOnboardSkillTemplate: 'bc2216b72724b01c3a733e63b8bf4aff457f561c0e9ff7288bdacc39780a37a7', getOpsxExploreCommandTemplate: 'eef1f8b4fd90ade6d70be46f0f8c3e6722f221fed175a6f9cf626287ef504a94', getOpsxNewCommandTemplate: '57c600cce318d16b9b4308a18d0d983ea3c0673034e606a7cceec07b4c705e87', getOpsxContinueCommandTemplate: '7843e40ad80611a80bcd3c8c5abd5ce7f89efe72f749a482fd1d0594762e94f3', getOpsxApplyCommandTemplate: 'daeb507206707169de73c828e199648dde5732cbc17791ef2a027adffd028574', - getOpsxFfCommandTemplate: 'd2d8ea4f6ebf68fb591ce45796aa62387c6c40030360963fae0589fb003c559f', + getOpsxFfCommandTemplate: '81686b8e26e61167874d696c905102a6996cd664166003c9c71c511b41e00da6', getArchiveChangeSkillTemplate: 'b04eccde2c57af4bc484fa7279fa873ad1d46474eb024467d68e784d8b985c18', getBulkArchiveChangeSkillTemplate: '2b74b1f73380ff32e35f580734780d843c6161a2748c39edb07f1e00453771b4', getOpsxSyncCommandTemplate: '68dc44c9be2ec1ef719a4ed59830e5a0bc74c3ba6113070650266e1b0d153071', @@ -57,8 +57,8 @@ const EXPECTED_FUNCTION_HASHES: Record = { getOpsxOnboardCommandTemplate: '9430a0fb6530791ab720e068f4b172bc3dfc4e96a1ae29102bee0b92c2afe7b5', getOpsxBulkArchiveCommandTemplate: 'da7be1a7318f15b915f5aae8eb638797a8a24a31e5fc7fc0a2bad01bba137686', getOpsxVerifyCommandTemplate: 'f01c0c0cef53be0956de52363d955d4ace131b1b2d77adf902f35fead9a1486d', - getOpsxProposeSkillTemplate: '1cb094f058e884aa8ddacd2ea756e4985bfb56b60628ca680e00cc0bdb97101d', - getOpsxProposeCommandTemplate: '494cfbe3a10510d356b513969481541088108123107562a6d4e2f0592ab9db34', + getOpsxProposeSkillTemplate: '7935c0be966667308c9d6abb5fc05058872233d5ae9f2d83e0a2015f4e9c4ef9', + getOpsxProposeCommandTemplate: 'c78b8521893b43398ef30477f03d1235d5229304929574a27abe500f33495687', getFeedbackSkillTemplate: 'd7d83c5f7fc2b92fe8f4588a5bf2d9cb315e4c73ec19bcd5ef28270906319a0d', getUpdateChangeSkillTemplate: 'fe2e8edaf973d42dc7fc7dfd846105c4c3cfec0437606e582ec644985cd4e81d', getOpsxUpdateCommandTemplate: 'e55ac5774203a7d9037d2d588889c97c53f3f930da49497cc79e865375920da7', @@ -69,13 +69,13 @@ const EXPECTED_GENERATED_SKILL_CONTENT_HASHES: Record = { 'openspec-new-change': 'd5b8909bea70a33b7a312b38ce204a91f40b6bb2bff12c4c06b3e11641b6a689', 'openspec-continue-change': '30b074eec5f1e70bba3a71d50175dbbcb2994a64930cb4cfd1660872ba767018', 'openspec-apply-change': '09c0e1cdf5ccc82416d0969d6bd715cc70616bdbc3531358a5c36057f78be55a', - 'openspec-ff-change': 'fa41b10a3101ba58742f9c15fe35f843c1ebcb94c7e4898bd7feaadd6676a80c', + 'openspec-ff-change': '449a0bccab74183791f4a981cfb563b90b752d5c91368f951550f333d0f6ffb4', 'openspec-sync-specs': 'd1bcd420bf8fb55a13f58a2857e6ebde58eb6f9e721a3bf6876bd9f640a63859', 'openspec-archive-change': 'b24d326662ef58809de4464960440713748b9a281323357facdca24af52014e7', 'openspec-bulk-archive-change': '49d410bda408c0411decd584be9c2355335e3b3db760fc6a0adcd82c172a280f', 'openspec-verify-change': '57693d22940f06080c6cf8d590ac2f48240d4a5e9ce7074dacd0f8d3c9945afa', 'openspec-onboard': '76225d10352454a304e56566997811d16f91de1b37653816f2bc5d8ec976febc', - 'openspec-propose': '69329d1eaacfff230d8641809e0290c8b501055ce301a8b76d4c044f42e1fec2', + 'openspec-propose': 'f858b1be2b64ce744d2ff0c8be43aa90c6b2f37e40ca1794276ff7859898111f', 'openspec-update-change': '77ff4d1f1cd08a57649cce1f25e0ebc4f55d6d032dfde5c301d1b479561b72fa', };