fix(templates): give explore the project's context and rules - #1408
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughExplore now loads ChangesExplore context loading
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
|
Ran three independent adversarial review passes over this branch (runtime correctness, regression risk, design fit) before asking for review. Recording what they found and what I did, so a reviewer doesn't have to re-derive it. Fixed in 33d9a1e1.
2. 3. Leakage wording was aimed at the wrong target. The original said "not material to recite back to the user" — but house style everywhere else ( Checked and found clean
Raised, deliberately not done hereThe design-fit pass made a fair point: no other workflow template reads an OpenSpec-owned config file directly — they all go through a CLI call. The clean answer is to have |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/core/templates/explore.test.ts`:
- Around line 22-36: Strengthen the assertions in the tests covering startup
config loading so they verify ordering, not just independent substrings: use
indexOf(...) to assert that “openspec list --json” and root.path resolution
occur before the openspec/config.yaml instructions. Update the relevant test
cases in the explore template suite while preserving the existing checks and run
the specified Vitest test.
🪄 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: e21ee40f-43e0-4cf7-89a2-21169ffa53f5
📒 Files selected for processing (5)
.changeset/explore-project-context.mdskills/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 (4)
- src/core/templates/workflows/explore.ts
- .changeset/explore-project-context.md
- skills/openspec-explore/SKILL.md
- test/core/templates/skill-templates-parity.test.ts
| it('loads project context from the OpenSpec config at startup (#696)', () => { | ||
| for (const [label, body] of bodies) { | ||
| expect(body, label).toContain('openspec/config.yaml'); | ||
| expect(body, label).toContain('`context`: project background'); | ||
| expect(body, label).toContain('`rules`: keyed by artifact id'); | ||
| } | ||
| }); | ||
|
|
||
| it('resolves the config through the reported root rather than assuming a repo-local path (#696)', () => { | ||
| for (const [label, body] of bodies) { | ||
| expect(body, label).toContain('openspec list --json'); | ||
| expect(body, label).toContain('<root.path>/openspec/config.yaml'); | ||
| expect(body, label).toContain('root.path'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that config loading follows openspec list --json.
These assertions only verify independent substrings; they would pass if the config instructions appeared before root resolution or were unrelated to the openspec list --json step. Assert the relative ordering using indexOf(...) so the startup contract is protected.
+ const listIndex = body.indexOf('openspec list --json');
+ const configIndex = body.indexOf('<root.path>/openspec/config.yaml');
+ expect(configIndex).toBeGreaterThan(listIndex);Run with pnpm exec vitest run test/core/templates/explore.test.ts.
📝 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.
| it('loads project context from the OpenSpec config at startup (#696)', () => { | |
| for (const [label, body] of bodies) { | |
| expect(body, label).toContain('openspec/config.yaml'); | |
| expect(body, label).toContain('`context`: project background'); | |
| expect(body, label).toContain('`rules`: keyed by artifact id'); | |
| } | |
| }); | |
| it('resolves the config through the reported root rather than assuming a repo-local path (#696)', () => { | |
| for (const [label, body] of bodies) { | |
| expect(body, label).toContain('openspec list --json'); | |
| expect(body, label).toContain('<root.path>/openspec/config.yaml'); | |
| expect(body, label).toContain('root.path'); | |
| } | |
| }); | |
| it('loads project context from the OpenSpec config at startup (`#696`)', () => { | |
| for (const [label, body] of bodies) { | |
| expect(body, label).toContain('openspec/config.yaml'); | |
| expect(body, label).toContain('`context`: project background'); | |
| expect(body, label).toContain('`rules`: keyed by artifact id'); | |
| } | |
| }); | |
| it('resolves the config through the reported root rather than assuming a repo-local path (`#696`)', () => { | |
| for (const [label, body] of bodies) { | |
| expect(body, label).toContain('openspec list --json'); | |
| expect(body, label).toContain('<root.path>/openspec/config.yaml'); | |
| expect(body, label).toContain('root.path'); | |
| const listIndex = body.indexOf('openspec list --json'); | |
| const configIndex = body.indexOf('<root.path>/openspec/config.yaml'); | |
| expect(configIndex).toBeGreaterThan(listIndex); | |
| } | |
| }); |
🤖 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 22 - 36, Strengthen the
assertions in the tests covering startup config loading so they verify ordering,
not just independent substrings: use indexOf(...) to assert that “openspec list
--json” and root.path resolution occur before the openspec/config.yaml
instructions. Update the relevant test cases in the explore template suite while
preserving the existing checks and run the specified Vitest test.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified at 9633988: the resolved-root config guidance is scoped correctly across both delivery surfaces, and the focused template/parity tests plus a clean regeneration pass locally. The ordering-test nit is non-blocking because the generated instructions themselves have the required order.
Explore was the only workflow that never loaded openspec/config.yaml. Every artifact-creating workflow receives the project's `context` and `rules` through `openspec instructions --json`, but explore has no artifact or change name, so it never travels that path — it started a session knowing only what `openspec list --json` returns. The result was a thinking partner blind to the project's own tech stack, conventions, and constraints. Both the skill and command surfaces now read the config through the `root.path` reported by `openspec list --json`, so stores and workspace planning homes resolve correctly instead of assuming a repo-local path. Guidance-only: no CLI behavior, schema, or architecture changes. Fixes #696 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-ups on the explore context guidance: - `config.yml` is a first-class alternative to `config.yaml` (`resolveConfigFilePath` probes both, and `init` leaves a `.yml` project on `.yml` permanently). Naming only `.yaml` meant those projects hit the skip-if-missing branch and silently lost their context - the exact failure this change set out to fix. - `rules` is keyed by artifact id, and explore holds no artifact at startup. The guidance now says the entries apply when writing that artifact, so rules for one artifact are not applied to another. - Match house style on leakage: every sibling template and the instructions renderer forbid copying context/rules into the artifact, not just into the conversation. The wording now covers both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9633988 to
bb6e1f0
Compare
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The rebase preserves the reviewed resolved-root config guidance, config.yml handling, artifact-scoped rules, and non-leakage boundary. Exact-head build, clean skill regeneration, 15 focused tests, and the full CI/CodeQL matrix pass.
2 participants
Fixes #696.
Status: LGTM — guidance-only change to the explore skill and command templates. No CLI behavior, schema, parser, or architecture changes. Full suite green (2,032 passed; the only failures are the 17 known environment-only
zsh-installerones addressed by #1400).What was wrong
Explore was the only workflow that never saw the project's own context.
Every artifact-creating workflow (
propose,continue,ff, …) gets thecontextandrulesfrom the OpenSpec config injected for free, because they all callopenspec instructions <artifact-id> --change "<name>" --json, which reads the project config internally.Explore has no artifact and often no change name, so it never travels that path. At session start it ran only:
That returns active changes and nothing else. So explore — the step where you do your thinking, before anything is written down — was blind to the project's declared tech stack, conventions, and constraints. It would happily reason toward a suggestion the project's own config rules out.
How it was fixed
After the existing
openspec list --jsoncall, both the skill and the command surface now read the project's context:Four deliberate details:
root.path, not a hardcoded path.openspec list --jsonalready reports the resolved root, so this works for stores and workspace planning homes.<root.path>/openspec/config.*is exactly whatinstructions.tsdoes viareadProjectConfig(root.path)for every other workflow.resolveConfigFilePathprobesconfig.yamlthenconfig.yml, andinitreturns'exists'for either — so a.ymlproject stays.ymlforever. Naming only.yamlwould have silently skipped context for those projects.rulesisRecord<artifactId, string[]>; explore holds no artifact at startup, so the guidance is explicit that entries apply when writing that artifact.Proof it works
New focused test,
test/core/templates/explore.test.ts— every assertion runs against both delivery surfaces (skill and command), since the two bodies must stay in contract:Reverting the template change fails all five. Regenerated
skills/openspec-explore/SKILL.mdviapnpm generate:skills(verified byte-identical, generator is idempotent) and refreshed the three golden hashes inskill-templates-parity.test.ts, per the repo's template-change convention.Notes / nits
explore.tsbecause that file carries the skill body and the command body separately — matching how every other workflow template in this directory is structured.openspec context --jsonalready loads the parsed project config and discards it (context.ts→gatherRelationshipData), but currently emits onlyroot/members/status. Emittingcontextandrulesthere would let this template sayopenspec context --jsonlike every other template uses a CLI call, instead of naming a config path. That is a public JSON-output contract change and a maintainer call, so it is not in this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
openspec/config.yaml(orconfig.yml) at startup./opsx:explorecommand.Bug Fixes
Documentation
Tests
config.ymlsupport, and correct root-path resolution.