fix(explore): scaffold changes before capturing artifacts - #1503
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExplore mode now supports capturing accepted exploration as a scaffolded OpenSpec change. The guidance requires ChangesExplore change capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExploreWorkflow
participant OpenSpecCLI
participant ChangeArtifacts
ExploreWorkflow->>OpenSpecCLI: Run openspec new change
OpenSpecCLI-->>ExploreWorkflow: Return scaffolded change and .openspec.yaml
ExploreWorkflow->>OpenSpecCLI: Query status and instructions
OpenSpecCLI-->>ExploreWorkflow: Return artifact order and context
ExploreWorkflow->>ChangeArtifacts: Create requested artifacts
ExploreWorkflow->>OpenSpecCLI: Refresh status and verify outputs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/templates/explore.test.ts (1)
82-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert workflow order and branch coverage.
These checks verify that required phrases exist, but not their order or repetition. A future edit could move
openspec instructionsbefore the initial status query, remove the status refresh, or drop the start-only and blocked-prerequisite behavior while the test remains green. Assert the sequence and occurrence counts for scaffold, status, instructions, output verification, and status refresh. Add coverage for the two branch conditions.As per coding guidelines, run focused file testing with
pnpm exec vitest run test/core/templates/explore.test.ts.Also applies to: 101-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/templates/explore.test.ts` around lines 82 - 99, Strengthen the tests around the bodies loop in the accepted-transition test to assert workflow ordering and occurrence counts for scaffold, initial status, instructions, output verification, and status refresh rather than only phrase presence. Add focused coverage for start-only and blocked-prerequisite branches, and run the file with pnpm exec vitest run test/core/templates/explore.test.ts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/core/templates/explore.test.ts`:
- Around line 82-99: Strengthen the tests around the bodies loop in the
accepted-transition test to assert workflow ordering and occurrence counts for
scaffold, initial status, instructions, output verification, and status refresh
rather than only phrase presence. Add focused coverage for start-only and
blocked-prerequisite branches, and run the file with pnpm exec vitest run
test/core/templates/explore.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2fc9466f-c23d-4785-a815-932aca027eef
📒 Files selected for processing (5)
.changeset/scaffold-explore-transitions.mdskills/openspec-explore/SKILL.mdsrc/core/templates/workflows/explore.tstest/core/templates/explore.test.tstest/core/templates/skill-templates-parity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Clean fix. The generated explore guidance now scaffolds metadata before capture, preserves store selection, and follows dependency-aware status/instructions without turning exploration into implementation. Full CI is green.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The conditional-prerequisite refinement looks good, but this commit also dropped the guidance and test that keep the selected --store on follow-up status and instructions calls. From a directory with only a registered store, the generated sequence now scaffolds the change in that store and then fails immediately at status with no_root_with_registered_stores; please restore store propagation and its regression assertion for every applicable follow-up command.
|
Addressed the outstanding review feedback in 20b938f:
Verification: 3,481/3,481 tests passed across 119 test files; focused explore/parity tests, ESLint, TypeScript build, and |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/openspec-explore/SKILL.md`:
- Around line 111-114: Update the command examples in the openspec workflow
instructions so every follow-up status and instructions invocation propagates
the confirmed store ID when a standalone store was selected, while local-root
examples omit any unbound --store placeholder. Apply the same conditional store
wording to the dependency instructions call and the status refresh in step 4,
then update the generator or template and regenerate SKILL.md.
🪄 Autofix
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 Plus
Run ID: 5b858bdc-b1b6-4f0c-aaf3-d020e1f3c206
📒 Files selected for processing (4)
skills/openspec-explore/SKILL.mdsrc/core/templates/workflows/explore.tstest/core/templates/explore.test.tstest/core/templates/skill-templates-parity.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/templates/workflows/explore.ts
- test/core/templates/skill-templates-parity.test.ts
- test/core/templates/explore.test.ts
Deploying openspec-docs with
|
| Latest commit: |
af9834a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ecb920ae.openspec-docs.pages.dev |
| Branch Preview URL: | https://codex-scaffold-explore-trans.openspec-docs.pages.dev |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Exact head ebf13c5 is clean. The selected standalone store now propagates through all five applicable status and instructions follow-ups while local-root commands remain unqualified; a fresh build and 31 focused explore/parity tests pass, with hosted CI green.
af9834a to
827501a
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased head. The explore capture transition still scaffolds metadata first, respects conditional prerequisites, and preserves a selected standalone store across every applicable follow-up command. A fresh build and all 35 focused explore/parity tests pass, and the full hosted matrix is green.
2 participants
Status
LGTM.
What was wrong
Explore mode could offer to capture an idea as a proposal without first scaffolding a change. An agent could create files directly under
openspec/changes/, omit the required.openspec.yaml, or make the user invoke another workflow command to finish the capture.How it was fixed
openspec new changebefore any new-change artifacts are written and retain the selected store on follow-up commands.statusand artifactinstructions, in dependency order.This changes generated guidance only. It does not change CLI behavior, APIs, schemas, persisted data, or architecture.
Replication / proof
The new regression assertions failed on current
mainbecause neither explore delivery surface required scaffolding or carried the transition through artifact creation. They pass with this patch.git diff --checkpassed.Notes / nits
Open PRs #1017 and #1500 touch the same template/parity or generated-skill files for unrelated guidance, so whichever lands second may need a mechanical rebase.
CodeRabbit linked-issue note: the additional ideas discussed in #720 are intentionally out of scope. The issue follow-up explains that
rules.designis artifact-scoped and cannot guard Explore, while an FF metadata check would be an optional defensive follow-up. This PR fixes the reported failure at its source by requiringopenspec new changebefore Explore captures artifacts, which creates.openspec.yaml.Closes #668.
Closes #720.
Summary by CodeRabbit