Fix apply instructions for glob artifact outputs#967
Merged
Conversation
Contributor
|
Task completed. I'll review this pull request by first examining the changes in detail. Powered by 1Code |
Contributor
📝 WalkthroughWalkthroughRefactors artifact output resolution by centralizing glob detection/resolution (resolveArtifactOutputs, artifactOutputExists, isGlobPattern) and changes apply instructions’ Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (instructions apply)
participant Instr as Instructions Generator
participant AG as Artifact Graph (resolveArtifactOutputs)
participant FS as Filesystem
CLI->>Instr: request apply instructions for change
Instr->>AG: resolveArtifactOutputs(changeDir, artifact.generates)
AG->>FS: glob/stat checks
FS-->>AG: matched paths[]
AG-->>Instr: returns paths[] per artifact
Instr-->>CLI: respond with contextFiles {artifactId: [paths...] }
CLI->>CLI: print / JSON-emit each artifact path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
src/commands/workflow/instructions.ts (1)
266-280: Resolve artifact outputs once and reuse them.
resolveArtifactOutputsis now doing the same filesystem walk twice per artifact: once formissingArtifactsand again forcontextFiles. Caching the resolved outputs in one pass is cheaper and keeps both structures derived from the same snapshot.♻️ Suggested refactor
- const missingArtifacts: string[] = []; - for (const artifactId of requiredArtifactIds) { - const artifact = schema.artifacts.find((a) => a.id === artifactId); - if (artifact && resolveArtifactOutputs(changeDir, artifact.generates).length === 0) { - missingArtifacts.push(artifactId); - } - } - - // Build context files from all existing artifacts in schema - const contextFiles: Record<string, string[]> = {}; - for (const artifact of schema.artifacts) { - const outputs = resolveArtifactOutputs(changeDir, artifact.generates); - if (outputs.length > 0) { - contextFiles[artifact.id] = outputs; - } - } + const outputsByArtifact = new Map( + schema.artifacts.map((artifact) => [ + artifact.id, + resolveArtifactOutputs(changeDir, artifact.generates), + ]) + ); + + const missingArtifacts = requiredArtifactIds.filter((artifactId) => { + const outputs = outputsByArtifact.get(artifactId); + return outputs !== undefined && outputs.length === 0; + }); + + // Build context files from all existing artifacts in schema + const contextFiles: Record<string, string[]> = {}; + for (const [artifactId, outputs] of outputsByArtifact) { + if (outputs.length > 0) { + contextFiles[artifactId] = outputs; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/workflow/instructions.ts` around lines 266 - 280, The code calls resolveArtifactOutputs twice per artifact; change the loop so you compute outputs once and reuse them: iterate schema.artifacts, for each artifact call resolveArtifactOutputs(changeDir, artifact.generates) once, store the result in a local map (e.g. outputsByArtifactId), then use that cached outputs to populate missingArtifacts (push artifact.id if requiredArtifactIds contains it and outputs.length === 0) and to build contextFiles (set contextFiles[artifact.id] = outputs if outputs.length > 0); reference symbols: resolveArtifactOutputs, schema.artifacts, artifact.generates, missingArtifacts, contextFiles.test/commands/artifact-workflow.test.ts (1)
450-480: Exercise multiple glob matches end-to-end here.This test still produces only one
specs/*/spec.mdmatch, so a regression that accidentally truncatescontextFiles.specsto the first resolved file would still pass. Adding a second matching file would better cover the new array contract.🧪 Suggested test hardening
- const specPath = path.join(changeDir, 'specs', 'single-star-glob', 'spec.md'); - await fs.mkdir(path.dirname(specPath), { recursive: true }); + const specPathA = path.join(changeDir, 'specs', 'single-star-glob', 'spec.md'); + const specPathB = path.join(changeDir, 'specs', 'another-match', 'spec.md'); + await fs.mkdir(path.dirname(specPathA), { recursive: true }); + await fs.mkdir(path.dirname(specPathB), { recursive: true }); await fs.writeFile(path.join(changeDir, '.openspec.yaml'), 'schema: glob-test\n'); - await fs.writeFile(specPath, '# Nested spec\n'); + await fs.writeFile(specPathA, '# Nested spec A\n'); + await fs.writeFile(specPathB, '# Nested spec B\n'); ... - const resolvedSpecPath = await fs.realpath(specPath); + const resolvedSpecPaths = await Promise.all([ + fs.realpath(specPathA), + fs.realpath(specPathB), + ]); expect(applyJson.state).toBe('ready'); expect(applyJson.missingArtifacts).toBeUndefined(); expect(applyJson.contextFiles).toEqual({ - specs: [resolvedSpecPath], + specs: expect.arrayContaining(resolvedSpecPaths), }); + expect(applyJson.contextFiles.specs).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/artifact-workflow.test.ts` around lines 450 - 480, The test only creates one matching specs/*/spec.md so it can't catch regressions that trim contextFiles.specs to a single entry; update the test around changeDir/specs to create a second matching spec (e.g., a sibling directory with its own spec.md), write both files and compute both realpaths, then assert that statusJson.artifacts still contains the glob and that applyJson.contextFiles.specs is an array containing both resolved spec paths (order-insensitive or sorted) instead of a single path; use the existing variables (changeDir, specPath, runCLI, statusResult, applyResult) and compare against the two resolved paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/artifact-graph/outputs.ts`:
- Around line 20-22: The non-glob branch currently treats any existing path as a
valid output; change it to enforce file-only semantics like the glob branch. In
the block that checks isGlobPattern(generates) and returns
fs.existsSync(fullPattern) ? [fullPattern] : [], replace the existence check
with a file-only check (e.g., verify fs.existsSync(fullPattern) &&
fs.lstatSync(fullPattern).isFile()) so directories are not treated as completed
outputs; keep the same return shape ([fullPattern] or []). Ensure you reference
the variables/methods isGlobPattern, generates, and fullPattern when making the
change.
---
Nitpick comments:
In `@src/commands/workflow/instructions.ts`:
- Around line 266-280: The code calls resolveArtifactOutputs twice per artifact;
change the loop so you compute outputs once and reuse them: iterate
schema.artifacts, for each artifact call resolveArtifactOutputs(changeDir,
artifact.generates) once, store the result in a local map (e.g.
outputsByArtifactId), then use that cached outputs to populate missingArtifacts
(push artifact.id if requiredArtifactIds contains it and outputs.length === 0)
and to build contextFiles (set contextFiles[artifact.id] = outputs if
outputs.length > 0); reference symbols: resolveArtifactOutputs,
schema.artifacts, artifact.generates, missingArtifacts, contextFiles.
In `@test/commands/artifact-workflow.test.ts`:
- Around line 450-480: The test only creates one matching specs/*/spec.md so it
can't catch regressions that trim contextFiles.specs to a single entry; update
the test around changeDir/specs to create a second matching spec (e.g., a
sibling directory with its own spec.md), write both files and compute both
realpaths, then assert that statusJson.artifacts still contains the glob and
that applyJson.contextFiles.specs is an array containing both resolved spec
paths (order-insensitive or sorted) instead of a single path; use the existing
variables (changeDir, specPath, runCLI, statusResult, applyResult) and compare
against the two resolved paths.
🪄 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: 18306353-214a-456d-a847-94c7424f7c06
📒 Files selected for processing (11)
openspec/specs/cli-artifact-workflow/spec.mdsrc/commands/workflow/instructions.tssrc/commands/workflow/shared.tssrc/core/artifact-graph/index.tssrc/core/artifact-graph/outputs.tssrc/core/artifact-graph/state.tssrc/core/templates/workflows/apply-change.tssrc/core/templates/workflows/verify-change.tstest/commands/artifact-workflow.test.tstest/core/artifact-graph/outputs.test.tstest/core/templates/skill-templates-parity.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/artifact-graph/outputs.ts (1)
9-11: ReplaceisGlobPatternwithfg.isDynamicPattern.The manual check using simple
includes()can miss edge cases like escaped characters (\*), brace expansions, and extglobs thatfast-globitself recognizes. Usingfg.isDynamicPattern()directly ensures pattern detection aligns with actual glob behavior on line 29.export function isGlobPattern(pattern: string): boolean { return fg.isDynamicPattern(pattern); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/artifact-graph/outputs.ts` around lines 9 - 11, Replace the manual pattern check in isGlobPattern with fast-glob's canonical detector: change isGlobPattern to call fg.isDynamicPattern(pattern) so it correctly handles escaped characters, brace expansions and extglobs; also ensure the module importing fast-glob (fg) is present or add "import fg from 'fast-glob'" at the top if missing so isGlobPattern uses fg.isDynamicPattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/artifact-graph/outputs.ts`:
- Around line 9-11: Replace the manual pattern check in isGlobPattern with
fast-glob's canonical detector: change isGlobPattern to call
fg.isDynamicPattern(pattern) so it correctly handles escaped characters, brace
expansions and extglobs; also ensure the module importing fast-glob (fg) is
present or add "import fg from 'fast-glob'" at the top if missing so
isGlobPattern uses fg.isDynamicPattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac9787ca-e36d-4a95-a9eb-8a6cee81bf8c
📒 Files selected for processing (2)
src/core/artifact-graph/outputs.tstest/core/artifact-graph/outputs.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/core/artifact-graph/outputs.test.ts
alfred-openspec
approved these changes
Apr 12, 2026
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Looks good. Centralizing artifact output resolution fixes the status/apply mismatch cleanly, and the regression coverage for nested globs plus concrete contextFiles makes me comfortable merging this.
This was referenced Apr 15, 2026
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
2 participants
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
Fix
openspec instructions applyso glob-generated artifacts resolve consistently withopenspec status.Closes #942.
Problem
instructions applyused its own hand-written artifact matcher whilestatusused the shared artifact-graph completion logic backed byfast-glob.That caused two user-visible issues:
statuscould report a glob artifact as complete whileinstructions applystill blocked on itcontextFilescould contain raw glob patterns likespecs/**/*.mdinstead of real file pathsThe mismatch was especially visible for single-star nested patterns like
specs/*/spec.md.Changes
src/core/artifact-graph/outputs.tsinstructions applyto resolve concrete artifact outputs instead of using a custom matchercontextFilesto returnRecord<string, string[]>so glob artifacts can expose all concrete matched filesspecs/*/spec.md)?,[], basename-sensitive patterns)contextFilesoutput for built-in spec-driven schemasVerification
pnpm run buildnode "$(node -p "require.resolve('vitest/vitest.mjs')")" runSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests