Conversation
Adds support for reading structured specs from OpenSpec directories. OpenSpec organizes specs into changes (proposal, design, tasks, specs) which map naturally to ralph-starter's source architecture. Usage: ralph-starter run --from openspec # list changes ralph-starter run --from openspec:my-feat # specific change ralph-starter run --from openspec:all # all active changes ralph-starter run --from openspec:specs # global specs only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Issue Linking ReminderThis PR doesn't appear to have a linked issue. Consider linking to:
Using If this PR doesn't need an issue, you can ignore this message. |
✔️ Bundle Size Analysis
Bundle breakdown |
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — only P2 findings remain, all non-blocking. All three findings are P2: a fragile but currently working mock specifier, a metadata field that over-reports spec directories, and missing path-traversal sanitisation that has negligible impact in a local CLI. No logic errors, broken contracts, or data loss risks on the changed path. Test coverage is thorough (13 tests, all passing). src/sources/builtin/openspec.ts — minor metadata inconsistency and missing traversal guard worth cleaning up post-merge.
|
| Filename | Overview |
|---|---|
| src/sources/builtin/openspec.ts | New BuiltinSource that reads structured openspec/ directories; metadata.specs can include directories with no spec.md content and changeName lacks path-traversal guard. |
| src/sources/tests/openspec.test.ts | 13 tests covering all fetch modes; vi.mock targets 'fs' while source and test imports use 'node:fs' — currently works via Vitest aliasing but is fragile. |
| src/sources/index.ts | Two-line registration of OpenSpecSource in the builtin source list; clean and correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["fetch(identifier)"] --> B{id?}
B -- "empty / 'list'" --> C[listChanges]
B -- "'specs' / 'global'" --> D[fetchGlobalSpecs]
B -- "'all'" --> E[fetchAllChanges]
B -- "other" --> F[fetchChange]
C --> G[getActiveChanges\nopenspec/changes/*\nexclude archive]
E --> G
G --> H{changes\nexist?}
H -- yes --> I[SourceResult]
H -- no/empty --> J[error / empty result]
F --> K[join openspecDir/changes/changeName]
K --> L{dir exists?}
L -- no --> M[error]
L -- yes --> N[Read proposal.md\ndesign.md\ntasks.md\nspecs/*/spec.md]
N --> I
D --> O[join openspecDir/specs]
O --> P{exists?}
P -- no --> M
P -- yes --> Q[Read specs/*/spec.md]
Q --> I
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/sources/__tests__/openspec.test.ts
Line: 5-10
Comment:
**Mock specifier doesn't match import specifier**
The test file imports from `'node:fs'` (line 1) but the mock target is `'fs'`. Vitest currently aliases these, so the tests pass, but this could break if Vitest's module resolution changes. The source under test also uses `'node:fs'`, so aligning the mock is the safer approach.
```suggestion
vi.mock('node:fs', () => ({
existsSync: vi.fn(),
readFileSync: vi.fn(),
readdirSync: vi.fn(),
statSync: vi.fn(),
}));
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/sources/builtin/openspec.ts
Line: 254-263
Comment:
**`metadata.specs` includes directories without `spec.md`**
`specDirs` is built from all subdirectories, but only those with a `spec.md` are actually read into the output content. If a directory exists but has no `spec.md`, it shows up in `metadata.specs` but contributes nothing to `content`, making `specs.length` larger than `count`. A caller inspecting `metadata.specs` could conclude there is content for those directories when there isn't.
```suggestion
return {
content: sections.join('\n'),
source: 'openspec:specs',
title: 'OpenSpec Global Specs',
metadata: {
type: 'openspec',
mode: 'specs',
count: files.length,
specs: files.map((f) => f.replace('/spec.md', '').replace('specs/', '')),
},
};
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/sources/builtin/openspec.ts
Line: 126-130
Comment:
**`changeName` is used unsanitized in path construction**
`changeName` comes directly from the CLI identifier (e.g. `openspec:../../../etc`). `path.join` normalises `..` segments, so a name like `../../../outside` resolves to a directory outside `openspec/changes/`. The `isDirectory()` guard prevents reading arbitrary files, but allows stat/exists checks on arbitrary directories and could leak directory-existence information. Since this is a local CLI tool the practical risk is low, but adding a guard makes the intent explicit:
```suggestion
const changePath = join(openspecDir, 'changes', changeName);
// Prevent path traversal outside changes/
const changesRoot = join(openspecDir, 'changes');
if (!changePath.startsWith(changesRoot + '/') && changePath !== changesRoot) {
this.error(`Invalid change name: ${changeName}`);
}
if (!existsSync(changePath) || !statSync(changePath).isDirectory()) {
this.error(`Change not found: ${changeName}`);
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: add OpenSpec as builtin source for..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ea522246f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
openspec/directoryUsage
Files
src/sources/builtin/openspec.ts- OpenSpecSource implementationsrc/sources/__tests__/openspec.test.ts- 13 testssrc/sources/index.ts- Register in source registry (2 lines)Test plan
🤖 Generated with Claude Code