feat: add optional folder field on schema artifact definitions#1020
Open
harikrishnan83 wants to merge 3 commits intoFission-AI:mainfrom
Open
feat: add optional folder field on schema artifact definitions#1020harikrishnan83 wants to merge 3 commits intoFission-AI:mainfrom
harikrishnan83 wants to merge 3 commits intoFission-AI:mainfrom
Conversation
Lets schema authors re-parent artifacts (e.g. ADRs) to repo-root-relative locations outside openspec/changes/, with no archival and no delta format. Includes proposal, design, four delta specs (artifact-graph, cli-artifact-workflow, cli-archive, schema-validate-command), and a tasks checklist for implementation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reduce surface based on review: collapse path resolution into a single helper used by the existing resolveArtifactOutputs() chokepoint (no projectRoot threading), drop the archive preflight warning entirely (moveDirectory already cannot reach external paths), and rely on Node's stdlib path module instead of bespoke cross-platform handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds opt-in `folder:` on artifact schemas that re-parents an artifact's outputs to a project-root-relative directory (e.g., ADRs persisting past a single change). Implementation funnels through a single new `resolveArtifactBaseDir` helper inside `resolveArtifactOutputs()`, so state detection, instruction rendering, and archive behavior inherit the new resolution transitively. Validation runs in the Zod refinement layer (rejects empty/absolute/parent-traversal/`openspec/`-prefixed values) so every consumer sees the same errors.
Contributor
📝 WalkthroughWalkthroughThis PR adds support for an optional Changes
Sequence DiagramsequenceDiagram
participant SchemaAuthor as Schema Author
participant ParseLayer as Schema Parser
participant ValidateLayer as Folder Validator
participant ArtifactGraph as Artifact Graph
participant PathResolver as Path Resolver
participant InstructionGen as Instruction Generator
participant CliOutput as CLI Output
SchemaAuthor->>ParseLayer: Define artifact with folder: "docs/adr"
ParseLayer->>ValidateLayer: Pass folder value for validation
ValidateLayer->>ValidateLayer: Check non-empty, non-absolute, no traversal, no openspec/ prefix
ValidateLayer->>ArtifactGraph: Folder field stored in Artifact object
ArtifactGraph->>PathResolver: Request base directory for artifact
PathResolver->>PathResolver: Walk up changeDir to find projectRoot
PathResolver->>PathResolver: Resolve folder relative to projectRoot
PathResolver->>ArtifactGraph: Return external base directory
ArtifactGraph->>ArtifactGraph: Resolve generates glob against base directory
ArtifactGraph->>ArtifactGraph: Detect completion using external paths
InstructionGen->>PathResolver: Compute output directory
PathResolver->>InstructionGen: Return absolute external path
InstructionGen->>CliOutput: Render "Write to: /repo/docs/adr/..."
CliOutput->>SchemaAuthor: Display resolved external target location
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/core/artifact-graph/outputs.test.ts (1)
22-175: Add directfoldercases inoutputsunit tests.The refactor to artifact-shaped inputs is covered, but this file still lacks explicit assertions for
artifact.folderbehavior at theresolveArtifactOutputs/artifactOutputExistslevel.Diff suggestion
+ it('resolves outputs relative to project root when folder is set', () => { + const projectRoot = tempDir; + const changeDir = path.join(projectRoot, 'openspec', 'changes', 'my-change'); + const adrDir = path.join(projectRoot, 'ADR'); + const adrFile = path.join(adrDir, '0001.md'); + fs.mkdirSync(changeDir, { recursive: true }); + fs.mkdirSync(adrDir, { recursive: true }); + fs.writeFileSync(adrFile, 'content'); + + expect(resolveArtifactOutputs(artifactWith('*.md', 'ADR'), changeDir)).toEqual([canonical(adrFile)]); + expect(artifactOutputExists(artifactWith('*.md', 'ADR'), changeDir)).toBe(true); + }); + + it('returns false for folder-based artifact when external folder has no matches', () => { + const projectRoot = tempDir; + const changeDir = path.join(projectRoot, 'openspec', 'changes', 'my-change'); + fs.mkdirSync(changeDir, { recursive: true }); + fs.mkdirSync(path.join(projectRoot, 'ADR'), { recursive: true }); + + expect(resolveArtifactOutputs(artifactWith('*.md', 'ADR'), changeDir)).toEqual([]); + expect(artifactOutputExists(artifactWith('*.md', 'ADR'), changeDir)).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/artifact-graph/outputs.test.ts` around lines 22 - 175, The tests are missing cases that pass artifact.folder to resolveArtifactOutputs and artifactOutputExists; update the test suite to call artifactWith('proposal.md', '<folder>') (and similar glob cases) and assert behavior when folder is provided (e.g., a file exists in that folder resolves to canonical path, directories are not treated as files, and glob patterns resolve relative to the supplied folder) so that resolveArtifactOutputs and artifactOutputExists are validated with artifact.folder populated.test/core/artifact-graph/instruction-loader.test.ts (1)
211-232: Add one folder-aware dependency path assertion.This block validates
outputPath, but notdependencies[].path, which also changed to resolved absolute paths viaresolveArtifactBaseDir.Diff suggestion
+ it('resolves dependency path with external folder when dependency uses folder', () => { + createCustomSchema( + 'with-folder-deps', + `name: with-folder-deps +version: 1 +artifacts: + - id: adr + generates: "*.md" + folder: ADR + description: ADR + template: adr.md + requires: [] + - id: index + generates: index.md + description: Index + template: index.md + requires: [adr] +`, + { 'adr.md': '# ADR\n', 'index.md': '# Index\n' } + ); + + const context = loadChangeContext(tempDir, 'my-change', 'with-folder-deps'); + const instructions = generateInstructions(context, 'index', tempDir); + expect(instructions.dependencies[0].path).toBe(path.join(tempDir, 'ADR', '*.md')); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/artifact-graph/instruction-loader.test.ts` around lines 211 - 232, The test currently asserts only instructions.outputPath but must also verify that dependency paths were resolved with the artifact folder; add an assertion that checks instructions.dependencies[0].path equals the resolved absolute path for the dependency (use path.join(tempDir, 'ADR', <dependency filename or glob>) to match how resolveArtifactBaseDir is expected to resolve paths). Locate the test around the generateInstructions(context, 'adr', tempDir) call and add a folder-aware assertion referencing instructions and instructions.dependencies to ensure dependency.path was normalized using resolveArtifactBaseDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/customization.md`:
- Around line 224-227: The docs only list the reserved prefix rule for the
folder field; update the "Validation (enforced by `openspec schema validate`)"
section to enumerate all runtime folder validation rules: the folder value must
not be empty, must not be an absolute path (must not start with '/'), must not
contain parent-traversal components like '..' or any path segments that escape
the repo, and must still reject the reserved 'openspec/' prefix; mention these
checks are enforced by `openspec schema validate` and mirror the runtime
validation logic for clarity.
In `@openspec/changes/add-artifact-target-folder/design.md`:
- Line 3: The design doc references outdated function signatures for
resolveArtifactOutputs — update the mentions on Line 3 and Line 83 so they match
the current implementation that requires an artifact parameter (e.g.,
resolveArtifactOutputs(changeDir, artifact, generates)); ensure the doc text and
any example calls (including the `Write to:` display mention) reflect the new
required `artifact` argument and the exact parameter order/names used by the
implementation.
In `@openspec/changes/add-artifact-target-folder/specs/artifact-graph/spec.md`:
- Around line 28-29: The spec wording is too strict: change the sentence that
states "resolveArtifactOutputs() is the single production call site that uses
this helper" so it reflects reality that resolveArtifactOutputs() is a primary
production call site but not the only one; explicitly allow
resolveArtifactBaseDir to be used directly by instruction-loader.ts for
output/dependency path resolution and note that artifactOutputExists (in
state.ts), instructions.ts, and instruction-loader.ts all ultimately rely on
resolveArtifactBaseDir/resolveArtifactOutputs for folder: support. Update the
language to "primary" or "one of the production call sites" and list
resolveArtifactOutputs(), resolveArtifactBaseDir(), artifactOutputExists, and
instruction-loader.ts so the spec matches the implementation.
In
`@openspec/changes/add-artifact-target-folder/specs/schema-validate-command/spec.md`:
- Line 10: The spec's absolute-path validation currently relies on
path.isAbsolute(folder) which fails to detect Windows-style absolute paths when
running on POSIX: update the rule to check both platform-specific forms instead
of a single isAbsolute call — e.g., use path.posix.isAbsolute(folder) ||
path.win32.isAbsolute(folder) (or equivalent dual-check) where the validation of
the `folder` value occurs so both POSIX `/...` and Windows `C:\...` forms are
rejected.
In `@src/core/artifact-graph/schema.ts`:
- Around line 35-52: The code currently builds formattedPath from e.path and
emits `: <message>` when e.path is empty; update the formattedPath construction
in the same block (the logic that maps e.path and the variable formattedPath) to
detect an empty e.path and return a root fallback label (e.g., "root" or
"<root>") instead of producing an empty string, preserving existing segment
formatting for non-empty paths and leaving the final return `${formattedPath}:
${e.message}` unchanged; reference symbols: formattedPath, e.path,
parsedArtifacts.
---
Nitpick comments:
In `@test/core/artifact-graph/instruction-loader.test.ts`:
- Around line 211-232: The test currently asserts only instructions.outputPath
but must also verify that dependency paths were resolved with the artifact
folder; add an assertion that checks instructions.dependencies[0].path equals
the resolved absolute path for the dependency (use path.join(tempDir, 'ADR',
<dependency filename or glob>) to match how resolveArtifactBaseDir is expected
to resolve paths). Locate the test around the generateInstructions(context,
'adr', tempDir) call and add a folder-aware assertion referencing instructions
and instructions.dependencies to ensure dependency.path was normalized using
resolveArtifactBaseDir.
In `@test/core/artifact-graph/outputs.test.ts`:
- Around line 22-175: The tests are missing cases that pass artifact.folder to
resolveArtifactOutputs and artifactOutputExists; update the test suite to call
artifactWith('proposal.md', '<folder>') (and similar glob cases) and assert
behavior when folder is provided (e.g., a file exists in that folder resolves to
canonical path, directories are not treated as files, and glob patterns resolve
relative to the supplied folder) so that resolveArtifactOutputs and
artifactOutputExists are validated with artifact.folder populated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06765221-cd97-406f-a39a-e5b83c8702de
📒 Files selected for processing (22)
docs/customization.mdopenspec/changes/add-artifact-target-folder/.openspec.yamlopenspec/changes/add-artifact-target-folder/design.mdopenspec/changes/add-artifact-target-folder/proposal.mdopenspec/changes/add-artifact-target-folder/specs/artifact-graph/spec.mdopenspec/changes/add-artifact-target-folder/specs/cli-artifact-workflow/spec.mdopenspec/changes/add-artifact-target-folder/specs/schema-validate-command/spec.mdopenspec/changes/add-artifact-target-folder/tasks.mdsrc/commands/workflow/instructions.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/instruction-loader.tssrc/core/artifact-graph/outputs.tssrc/core/artifact-graph/paths.tssrc/core/artifact-graph/schema.tssrc/core/artifact-graph/state.tssrc/core/artifact-graph/types.tstest/commands/artifact-workflow.test.tstest/core/artifact-graph/instruction-loader.test.tstest/core/artifact-graph/outputs.test.tstest/core/artifact-graph/paths.test.tstest/core/artifact-graph/schema.test.tstest/core/artifact-graph/state.test.ts
Comment on lines
+224
to
+227
| **Validation** (enforced by `openspec schema validate`): | ||
|
|
||
| - Cannot start with the reserved `openspec/` prefix. | ||
|
|
Contributor
There was a problem hiding this comment.
Validation docs are incomplete for folder.
This section currently documents only the reserved prefix rule. It should also list empty/absolute/parent-traversal rejections to match runtime validation.
Suggested doc update
**Validation** (enforced by `openspec schema validate`):
+- Cannot be empty or whitespace-only.
+- Cannot be an absolute path.
+- Cannot traverse outside project root (no parent traversal escapes).
- Cannot start with the reserved `openspec/` prefix.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Validation** (enforced by `openspec schema validate`): | |
| - Cannot start with the reserved `openspec/` prefix. | |
| **Validation** (enforced by `openspec schema validate`): | |
| - Cannot be empty or whitespace-only. | |
| - Cannot be an absolute path. | |
| - Cannot traverse outside project root (no parent traversal escapes). | |
| - Cannot start with the reserved `openspec/` prefix. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/customization.md` around lines 224 - 227, The docs only list the
reserved prefix rule for the folder field; update the "Validation (enforced by
`openspec schema validate`)" section to enumerate all runtime folder validation
rules: the folder value must not be empty, must not be an absolute path (must
not start with '/'), must not contain parent-traversal components like '..' or
any path segments that escape the repo, and must still reject the reserved
'openspec/' prefix; mention these checks are enforced by `openspec schema
validate` and mirror the runtime validation logic for clarity.
| @@ -0,0 +1,128 @@ | |||
| ## Context | |||
|
|
|||
| Today every artifact in a schema is implicitly written under `openspec/changes/<change>/`, then moved into `openspec/changes/archive/<date>-<change>/` when the change is archived. All path resolution funnels through one function — `resolveArtifactOutputs(changeDir, generates)` at `src/core/artifact-graph/outputs.ts:19` — which `state.ts` (via `artifactOutputExists`) and `instructions.ts:269/277` both call. The display-only `Write to:` line at `instructions.ts:162/174` does its own `path.join(changeDir, …)` for the same value. | |||
Contributor
There was a problem hiding this comment.
Update stale function signatures in the design doc.
The signatures in Line 3 and Line 83 no longer match the implementation (artifact is now required). Please update these references so the design stays executable for future contributors.
Suggested doc fix
-All path resolution funnels through one function — `resolveArtifactOutputs(changeDir, generates)` at `src/core/artifact-graph/outputs.ts:19` ...
+All path resolution funnels through one function — `resolveArtifactOutputs(artifact, changeDir)` at `src/core/artifact-graph/outputs.ts:22` ...
-`artifactOutputExists(changeDir, generates)` already calls `resolveArtifactOutputs()` internally...
+`artifactOutputExists(artifact, changeDir)` already calls `resolveArtifactOutputs()` internally...Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/add-artifact-target-folder/design.md` at line 3, The design
doc references outdated function signatures for resolveArtifactOutputs — update
the mentions on Line 3 and Line 83 so they match the current implementation that
requires an artifact parameter (e.g., resolveArtifactOutputs(changeDir,
artifact, generates)); ensure the doc text and any example calls (including the
`Write to:` display mention) reflect the new required `artifact` argument and
the exact parameter order/names used by the implementation.
Comment on lines
+28
to
+29
| The existing `resolveArtifactOutputs()` function SHALL be the single production call site that uses this helper. Because `state.ts` (via `artifactOutputExists`) and `instructions.ts` already funnel through `resolveArtifactOutputs()`, completion detection and instruction rendering inherit `folder:` support transitively without changes of their own. | ||
|
|
Contributor
There was a problem hiding this comment.
Spec call-site statement is stricter than current implementation.
Line 28 says resolveArtifactOutputs() is the only production user of resolveArtifactBaseDir, but src/core/artifact-graph/instruction-loader.ts also calls the helper directly for output/dependency paths. Please relax this wording to match reality.
Suggested spec text update
-The existing `resolveArtifactOutputs()` function SHALL be the single production call site that uses this helper.
+`resolveArtifactOutputs()` SHALL use this helper for output existence/matching, and any direct path rendering code paths SHALL use the same helper to keep base-dir resolution consistent.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The existing `resolveArtifactOutputs()` function SHALL be the single production call site that uses this helper. Because `state.ts` (via `artifactOutputExists`) and `instructions.ts` already funnel through `resolveArtifactOutputs()`, completion detection and instruction rendering inherit `folder:` support transitively without changes of their own. | |
| `resolveArtifactOutputs()` SHALL use this helper for output existence/matching, and any direct path rendering code paths SHALL use the same helper to keep base-dir resolution consistent. Because `state.ts` (via `artifactOutputExists`) and `instructions.ts` already funnel through `resolveArtifactOutputs()`, completion detection and instruction rendering inherit `folder:` support transitively without changes of their own. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/add-artifact-target-folder/specs/artifact-graph/spec.md`
around lines 28 - 29, The spec wording is too strict: change the sentence that
states "resolveArtifactOutputs() is the single production call site that uses
this helper" so it reflects reality that resolveArtifactOutputs() is a primary
production call site but not the only one; explicitly allow
resolveArtifactBaseDir to be used directly by instruction-loader.ts for
output/dependency path resolution and note that artifactOutputExists (in
state.ts), instructions.ts, and instruction-loader.ts all ultimately rely on
resolveArtifactBaseDir/resolveArtifactOutputs for folder: support. Update the
language to "primary" or "one of the production call sites" and list
resolveArtifactOutputs(), resolveArtifactBaseDir(), artifactOutputExists, and
instruction-loader.ts so the spec matches the implementation.
| The rules SHALL be expressed using only Node's stdlib `path` module — no bespoke cross-platform parsing. The codebase's existing `FileSystemUtils` helpers cover any downstream cross-platform path math. | ||
|
|
||
| 1. The value MUST NOT be empty (after `trim()`). | ||
| 2. The value MUST NOT be an absolute path. A single `!path.isAbsolute(folder)` check natively covers POSIX `/...` and Windows `C:\...` forms — no separate Windows clause is required. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js path API docs, does path.isAbsolute("C:\\Windows\\Temp") evaluate as absolute on POSIX hosts, or should path.win32.isAbsolute be used for cross-platform schema validation?
💡 Result:
No, path.isAbsolute("C:\Windows\Temp") does not evaluate as absolute on POSIX hosts. On POSIX systems (like Linux/macOS), path.isAbsolute uses POSIX logic and only returns true for paths starting with '/'. A Windows path like "C:\Windows\Temp" starts with 'C', so it returns false. For cross-platform schema validation of Windows-style absolute paths, use path.win32.isAbsolute, which correctly identifies drive-letter paths like "C:\Windows\Temp" as absolute regardless of the host OS. path.isAbsolute is platform-dependent and delegates to the host's logic (path.posix on POSIX, path.win32 on Windows). The platform-specific variants (path.win32, path.posix) always use fixed logic and are designed for cross-platform path string manipulation.
Citations:
- 1: https://nodejs.org/docs/latest/api/path.html
- 2: https://nodejs.org/docs/v23.4.0/api/path.html
- 3: https://nodejs.org/api/path.html
- 4: https://stackoverflow.com/questions/21698906/how-to-check-if-a-path-is-absolute-or-relative/21698964
- 5: nodejs/node@c1364d15a2
Update the absolute-path validation rule to properly handle cross-platform forms.
Line 10 incorrectly states that a single path.isAbsolute(folder) check covers both POSIX and Windows absolute paths. This is not reliable: on POSIX hosts (Linux, macOS), path.isAbsolute("C:\\Windows\\Temp") returns false because the path does not start with /. For proper cross-platform schema validation, explicitly use both platform-specific checks, as shown in the suggested update below.
Suggested spec text update
-2. The value MUST NOT be an absolute path. A single `!path.isAbsolute(folder)` check natively covers POSIX `/...` and Windows `C:\...` forms — no separate Windows clause is required.
+2. The value MUST NOT be an absolute path. Validation MUST reject both POSIX and Windows absolute forms regardless of host OS (for example, using `path.posix.isAbsolute(...) || path.win32.isAbsolute(...)`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 2. The value MUST NOT be an absolute path. A single `!path.isAbsolute(folder)` check natively covers POSIX `/...` and Windows `C:\...` forms — no separate Windows clause is required. | |
| 2. The value MUST NOT be an absolute path. Validation MUST reject both POSIX and Windows absolute forms regardless of host OS (for example, using `path.posix.isAbsolute(...) || path.win32.isAbsolute(...)`). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/add-artifact-target-folder/specs/schema-validate-command/spec.md`
at line 10, The spec's absolute-path validation currently relies on
path.isAbsolute(folder) which fails to detect Windows-style absolute paths when
running on POSIX: update the rule to check both platform-specific forms instead
of a single isAbsolute call — e.g., use path.posix.isAbsolute(folder) ||
path.win32.isAbsolute(folder) (or equivalent dual-check) where the validation of
the `folder` value occurs so both POSIX `/...` and Windows `C:\...` forms are
rejected.
Comment on lines
+35
to
+52
| const formattedPath = e.path | ||
| .map((segment, idx) => { | ||
| if ( | ||
| idx === 1 && | ||
| e.path[0] === 'artifacts' && | ||
| typeof segment === 'number' && | ||
| parsedArtifacts[segment] && | ||
| typeof parsedArtifacts[segment].id === 'string' && | ||
| (parsedArtifacts[segment].id as string).length > 0 | ||
| ) { | ||
| return `[${parsedArtifacts[segment].id}]`; | ||
| } | ||
| return typeof segment === 'number' ? `[${segment}]` : `.${String(segment)}`; | ||
| }) | ||
| .join('') | ||
| .replace(/^\./, ''); | ||
| return `${formattedPath}: ${e.message}`; | ||
| }) |
Contributor
There was a problem hiding this comment.
Handle root-level validation errors explicitly.
If e.path is empty, Line [51] currently emits : <message>. Add a root fallback label for clearer errors.
Suggested fix
- return `${formattedPath}: ${e.message}`;
+ const pathLabel = formattedPath.length > 0 ? formattedPath : '<root>';
+ return `${pathLabel}: ${e.message}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const formattedPath = e.path | |
| .map((segment, idx) => { | |
| if ( | |
| idx === 1 && | |
| e.path[0] === 'artifacts' && | |
| typeof segment === 'number' && | |
| parsedArtifacts[segment] && | |
| typeof parsedArtifacts[segment].id === 'string' && | |
| (parsedArtifacts[segment].id as string).length > 0 | |
| ) { | |
| return `[${parsedArtifacts[segment].id}]`; | |
| } | |
| return typeof segment === 'number' ? `[${segment}]` : `.${String(segment)}`; | |
| }) | |
| .join('') | |
| .replace(/^\./, ''); | |
| return `${formattedPath}: ${e.message}`; | |
| }) | |
| const formattedPath = e.path | |
| .map((segment, idx) => { | |
| if ( | |
| idx === 1 && | |
| e.path[0] === 'artifacts' && | |
| typeof segment === 'number' && | |
| parsedArtifacts[segment] && | |
| typeof parsedArtifacts[segment].id === 'string' && | |
| (parsedArtifacts[segment].id as string).length > 0 | |
| ) { | |
| return `[${parsedArtifacts[segment].id}]`; | |
| } | |
| return typeof segment === 'number' ? `[${segment}]` : `.${String(segment)}`; | |
| }) | |
| .join('') | |
| .replace(/^\./, ''); | |
| const pathLabel = formattedPath.length > 0 ? formattedPath : '<root>'; | |
| return `${pathLabel}: ${e.message}`; | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/artifact-graph/schema.ts` around lines 35 - 52, The code currently
builds formattedPath from e.path and emits `: <message>` when e.path is empty;
update the formattedPath construction in the same block (the logic that maps
e.path and the variable formattedPath) to detect an empty e.path and return a
root fallback label (e.g., "root" or "<root>") instead of producing an empty
string, preserving existing segment formatting for non-empty paths and leaving
the final return `${formattedPath}: ${e.message}` unchanged; reference symbols:
formattedPath, e.path, parsedArtifacts.
harikrishnan83
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is in the context of custom schemas and the artifacts they generate.
Adds an opt-in
folder:field on artifact definitions in schema YAML. When set, the artifact's outputs are written to a project-root-relative directory instead of the change directory. This is useful for artifacts that should outlive a single change and don't belong inside the specs folder either (example: Architecture Decision Records).When
folder:is omitted, behavior is unchanged — every existing schema continues to work without modification.Motivation
Certain artifacts fit the following criteria:
openspec/— once archived they fall out of view, and they are not part of the spec source-of-truth either.Example: ADRs record decisions. If we only capture them in
design.md, once design is archived the decision is gone. Keeping ADRs in a dedicated top-level folder lets us reference them from future changes — both to align on current design choices and to remember historical decisions we don't want to revisit.Other examples: UI design choices, service boundaries, or changes where future contributors
need a persistent decision trail
Example schema
spec-driven-with-adr: https://github.com/intent-driven-dev/openspec-schemas/blob/main/openspec/schemas/spec-driven-with-adr/
exact line: https://github.com/intent-driven-dev/openspec-schemas/blob/f14d1d55ac484d3f2998d0f0449766676428cc3c/openspec/schemas/spec-driven-with-adr/schema.yaml#L206
Validation rules
The folder value must not start with the reserved
openspec/prefix, to avoid accidental collisions.Strictly additive change
The
folderfield is optional. Existing schemas (including built-inspec-driven) and existing changes are unaffected.Summary by CodeRabbit
New Features
folderproperty on artifacts to output files to project-root-relative directories instead of change directoriesDocumentation
Tests