feat: add openspec check command for static analysis integration#1105
Open
magicyuan876 wants to merge 1 commit into
Open
feat: add openspec check command for static analysis integration#1105magicyuan876 wants to merge 1 commit into
magicyuan876 wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a ChangesStatic Check Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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: 3
🧹 Nitpick comments (1)
src/commands/check.ts (1)
94-106: ⚡ Quick winKeep report ordering deterministic.
Line 95 appends by completion order, so check order can vary run-to-run. Store by
currentIndexinstead.💡 Suggested fix
- const results: CheckResult[] = []; + const results: CheckResult[] = new Array(queue.length); @@ task() .then((res) => { - results.push(res); + results[currentIndex] = res; }) .catch((error: any) => { const message = error?.message || 'Unknown error'; - results.push({ + results[currentIndex] = { name: checks[currentIndex]?.name ?? 'unknown', passed: false, durationMs: 0, stdout: '', stderr: message, issues: [{ level: 'ERROR', path: 'check', message }], }); }) @@ - const report: CheckReport = { + const orderedResults = results.filter((r): r is CheckResult => Boolean(r)); + const report: CheckReport = { changeName: resolvedChangeName, - checks: results, + checks: orderedResults, summary: { - total: results.length, - passed: results.filter((r) => r.passed && !r.skipped).length, - failed: results.filter((r) => !r.passed && !r.skipped).length, - skipped: results.filter((r) => r.skipped).length, + total: orderedResults.length, + passed: orderedResults.filter((r) => r.passed && !r.skipped).length, + failed: orderedResults.filter((r) => !r.passed && !r.skipped).length, + skipped: orderedResults.filter((r) => r.skipped).length, }, };Also applies to: 121-129
🤖 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 `@src/commands/check.ts` around lines 94 - 106, The code appends per-completion into results causing nondeterministic ordering; instead initialize results to the same length as checks and assign the result object at results[currentIndex] (use results[currentIndex] = { name: checks[currentIndex]?.name ?? 'unknown', passed: false, ... }) rather than results.push(...), and do the same replacement for the other catch block handling failures; ensure results is pre-sized (e.g., results = new Array(checks.length)) so later consumers see deterministic ordering by check index.
🤖 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 `@src/core/code-checker/runner.ts`:
- Around line 125-128: The timeout force-kill check uses child.killed which is
set to true immediately after sending SIGTERM, so SIGKILL never runs; update the
logic in runner.ts where the child process is escalated (the timeout callback
referencing child.kill) to instead check that the process is still running by
testing child.exitCode === null && child.signalCode === null before calling
child.kill('SIGKILL'), ensuring the SIGKILL fallback executes only when the
process hasn't actually exited.
- Around line 216-229: The glob-to-regex conversion in the block that builds
regexStr (using normalizedPattern, placeholders '{{GLOBSTAR}}' and '{{STAR}}',
and then producing regex/test against normalizedFile) only escapes dots and
leaves other regex metacharacters unescaped; update the logic to first
substitute ** and * with the existing placeholders, then escape all regex
metacharacters (+ ? ( ) [ ] { } | ^ $ . \ and similar) in regexStr, and finally
restore the placeholders to their intended regex fragments ('.*' and '[^/]*'),
then apply the existing anchoring and new RegExp; ensure you modify the code
surrounding regexStr replacement and placeholder handling rather than the
regex.test call.
In `@src/core/project-config.ts`:
- Around line 170-190: The current code validates the entire raw.checks array
with checksField.safeParse which fails the whole array if any single entry is
malformed; change to validate entries individually so valid checks are
preserved: define the check schema (e.g., checkSchema = z.object({name:...,
command:..., files:...})), ensure raw.checks is an array, then iterate
raw.checks and safeParse each item (or use checkSchema.safeParse) to collect
only the successfully parsed checks into config.checks, emit a warning for each
invalid item (instead of discarding the whole array), and keep references to
raw.checks, checksField/checkSchema, checksResult, and config.checks to locate
the logic to update.
---
Nitpick comments:
In `@src/commands/check.ts`:
- Around line 94-106: The code appends per-completion into results causing
nondeterministic ordering; instead initialize results to the same length as
checks and assign the result object at results[currentIndex] (use
results[currentIndex] = { name: checks[currentIndex]?.name ?? 'unknown', passed:
false, ... }) rather than results.push(...), and do the same replacement for the
other catch block handling failures; ensure results is pre-sized (e.g., results
= new Array(checks.length)) so later consumers see deterministic ordering by
check index.
🪄 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: b6871df5-afd1-4cdf-aa55-6de43734963e
📒 Files selected for processing (11)
openspec/config.yamlsrc/cli/index.tssrc/commands/check.tssrc/core/code-checker/detector.tssrc/core/code-checker/index.tssrc/core/code-checker/runner.tssrc/core/code-checker/types.tssrc/core/project-config.tstest/commands/check.test.tstest/core/code-checker/detector.test.tstest/core/code-checker/runner.test.ts
- Add src/core/code-checker/ module with runner, detector, and types - Add openspec check CLI command for lint/type-check/static analysis - Extend project config schema with checks array - Auto-detect checks for TS/JS, Rust, Python, Go, Ruby, Java - Support Java offline tools: Checkstyle, SpotBugs, PMD (Maven/Gradle) - Add file glob filtering, JSON output, concurrency control - Add comprehensive unit and E2E tests - Update dogfood config.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/code-checker/detector.test.ts (1)
23-30: ⚡ Quick winAdd coverage for Prettier in the TypeScript/JavaScript detection test.
This test currently validates only the TypeScript check, but the feature contract includes Prettier auto-detection too. Add a Prettier assertion here to protect against regressions in that path.
Suggested test addition
const result = detectChecks(testDir); expect(result).not.toBeNull(); expect(result!.detected.some((c) => c.name === 'TypeScript type check')).toBe(true); + expect(result!.detected.some((c) => /prettier/i.test(c.name))).toBe(true);🤖 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/code-checker/detector.test.ts` around lines 23 - 30, The test "detects TypeScript checks when package.json + tsconfig.json exist" calls detectChecks(testDir) but only asserts TypeScript detection; add an assertion that the Prettier check is also detected by verifying result!.detected contains an entry with name "Prettier" (e.g. expect(result!.detected.some(c => c.name === 'Prettier')).toBe(true)). Update the same test (function name detectChecks) to include this Prettier assertion immediately after the existing TypeScript assertion so the JS/TS Prettier auto-detection path is covered.
🤖 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/code-checker/detector.test.ts`:
- Around line 23-30: The test "detects TypeScript checks when package.json +
tsconfig.json exist" calls detectChecks(testDir) but only asserts TypeScript
detection; add an assertion that the Prettier check is also detected by
verifying result!.detected contains an entry with name "Prettier" (e.g.
expect(result!.detected.some(c => c.name === 'Prettier')).toBe(true)). Update
the same test (function name detectChecks) to include this Prettier assertion
immediately after the existing TypeScript assertion so the JS/TS Prettier
auto-detection path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d45afe1-3e5f-487a-a6b2-a9ebc220af67
📒 Files selected for processing (11)
openspec/config.yamlsrc/cli/index.tssrc/commands/check.tssrc/core/code-checker/detector.tssrc/core/code-checker/index.tssrc/core/code-checker/runner.tssrc/core/code-checker/types.tssrc/core/project-config.tstest/commands/check.test.tstest/core/code-checker/detector.test.tstest/core/code-checker/runner.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/core/code-checker/index.ts
- src/core/code-checker/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/cli/index.ts
- openspec/config.yaml
- src/commands/check.ts
- src/core/code-checker/detector.ts
- test/core/code-checker/runner.test.ts
- test/commands/check.test.ts
- src/core/project-config.ts
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 adds a new
openspec checkcommand that enables AI-generated code to be validated against language-specific static analysis tools (lint, type-check, format-check, etc.) before being considered "done".What's New
1.
openspec check [change-name]CLI command--jsonoutput for machine-readable reports--concurrencyfor parallel execution2.
checksconfiguration inopenspec/config.yaml3. Auto-detection (
detectChecks())Inspects project files and suggests appropriate checks for:
4. Core module (
src/core/code-checker/)runner.ts— executes shell commands with timeout, output parsing, file glob filteringdetector.ts— best-effort project type detectiontypes.ts— shared interfaces5. Tests
test/core/code-checker/runner.test.ts(8 tests)test/core/code-checker/detector.test.ts(14 tests)test/commands/check.test.ts(7 E2E tests)Why this helps
AI agents often generate code with type errors or lint violations that aren't caught until human review. By embedding
openspec checkinto the workflow, agents get structured, actionable feedback immediately — reducing token waste from iterative error-fixing loops.Summary by CodeRabbit
New Features
Tests