Skip to content

Conversation

@TabishB
Copy link
Contributor

@TabishB TabishB commented Jan 20, 2026

Summary

  • Add null check when parsing rules field in project config
  • YAML rules: with no value parses to null, and typeof null === 'object' in JavaScript
  • Without this check, Object.entries(null) would throw an error

Test plan

  • Added test case for rules: null scenario
  • Verified test passes with the fix
  • All existing tests still pass

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed project configuration parsing to properly handle empty or null rules fields without validation errors.

✏️ Tip: You can customize this high-level summary in your review settings.

Adds openspec/config.yaml support for project-level customization without
forking schemas. Teams can now:

- Set a default schema (used when --schema flag not provided)
- Inject project context into all artifact instructions
- Add per-artifact rules (e.g., proposal rules, specs rules)

Key changes:
- New `src/core/project-config.ts` with Zod schema and resilient parsing
- New `src/core/config-prompts.ts` for interactive config creation
- Updated schema resolution order: CLI → change metadata → config → default
- Updated instruction generation to inject <context> and <rules> XML sections
- Integrated config creation prompts into `artifact-experimental-setup`

Schema resolution precedence:
1. --schema CLI flag (explicit override)
2. .openspec.yaml in change directory (change-specific)
3. openspec/config.yaml schema field (project default)
4. "spec-driven" (hardcoded fallback)
…nfig

- Add project config integration tests in artifact-workflow.test.ts:
  - Test new change uses schema from config
  - Test CLI schema overrides config schema
  - Test context and rules injection in instructions
  - Test backwards compatibility without config file
  - Test immediate reflection of config changes

- Add performance benchmark tests in project-config.test.ts:
  - Typical config (1KB): <20ms target
  - Large config (50KB): <50ms target
  - Repeated reads consistency check
  - Missing config fast-path test

- Add project configuration documentation in experimental-workflow.md:
  - Config fields reference (schema, context, rules)
  - Schema precedence explanation
  - Artifact IDs by schema
  - Troubleshooting guide

- Mark all tasks complete in tasks.md
Remove performance benchmark tests from project-config.test.ts and
document the results directly in the source code instead. Benchmarks
showed config reads are fast enough (~0.5ms typical) that caching
is unnecessary.
- Remove nested <template> tags: generateInstructions() no longer wraps
  template content since printInstructionsText() already handles XML
  structure
- Fix schema message accuracy: newChangeCommand() now uses the resolved
  schema returned from createChange() instead of hardcoded fallback
- Add TTY check: artifact-experimental-setup skips interactive prompts
  in non-TTY environments (CI, automation) to prevent hangs
Add null check when parsing rules field since YAML `rules:` with no
value parses to null, and `typeof null === 'object'` in JavaScript.
Without this check, Object.entries(null) would throw an error.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026

📝 Walkthrough

Walkthrough

Adds a null guard to the rules field parsing in project-config.ts to prevent false positives when the rules field is null, since typeof null === 'object'. A corresponding test case validates that empty rules fields are parsed gracefully with appropriate warnings.

Changes

Cohort / File(s) Summary
Rules field null guard and test
src/core/project-config.ts, test/core/project-config.test.ts
Added null check before type validation of rules field to prevent false positives when raw.rules is null. New test case validates parsing of empty rules field (rules: with no value) produces warning without aborting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐰 A null that hides in shadows deep,
Now caught before it makes us weep!
With gentle guard and test so neat,
Our rules field validation's sweet. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a null guard for the rules field in project config to prevent errors from YAML parsing.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@vibe-kanban-cloud
Copy link

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

@greptile-apps
Copy link

greptile-apps bot commented Jan 20, 2026

Greptile Summary

Added null check when parsing the rules field in project config to prevent Object.entries(null) from throwing an error when YAML parses rules: with no value as null.

  • Fixed edge case where typeof null === 'object' in JavaScript could cause runtime error
  • Added comprehensive test case validating the fix
  • Config parsing now gracefully handles rules: null and continues processing other fields

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is a simple defensive null check that fixes a real edge case bug. The fix is minimal, well-tested, and follows JavaScript best practices for handling the quirk that typeof null === 'object'. The test case directly validates the bug scenario.
  • No files require special attention

Important Files Changed

Filename Overview
src/core/project-config.ts Added null check to prevent Object.entries(null) error when parsing YAML rules field
test/core/project-config.test.ts Added test case for rules: null scenario to validate the fix

Sequence Diagram

sequenceDiagram
    participant User
    participant readProjectConfig
    participant YAML Parser
    participant Validation Logic

    User->>readProjectConfig: Read config file
    readProjectConfig->>YAML Parser: Parse YAML content
    YAML Parser-->>readProjectConfig: Return parsed object (rules: null)
    
    readProjectConfig->>Validation Logic: Check raw.rules !== undefined
    Validation Logic-->>readProjectConfig: true
    
    readProjectConfig->>Validation Logic: Check typeof raw.rules === 'object'
    Validation Logic-->>readProjectConfig: true (typeof null === 'object')
    
    readProjectConfig->>Validation Logic: Check raw.rules !== null (NEW)
    Validation Logic-->>readProjectConfig: false (is null)
    
    readProjectConfig->>readProjectConfig: Skip Object.entries(null)
    readProjectConfig->>readProjectConfig: Log warning about invalid rules
    readProjectConfig-->>User: Return config with schema and context only
Loading

@TabishB TabishB merged commit 6c8c778 into main Jan 20, 2026
10 checks passed
@TabishB TabishB deleted the TabishB/schema-custom-audit branch January 20, 2026 07:49
StrayDragon pushed a commit to StrayDragon/OpenSpec that referenced this pull request Jan 20, 2026
* feat(config): add project-level configuration via openspec/config.yaml

Adds openspec/config.yaml support for project-level customization without
forking schemas. Teams can now:

- Set a default schema (used when --schema flag not provided)
- Inject project context into all artifact instructions
- Add per-artifact rules (e.g., proposal rules, specs rules)

Key changes:
- New `src/core/project-config.ts` with Zod schema and resilient parsing
- New `src/core/config-prompts.ts` for interactive config creation
- Updated schema resolution order: CLI → change metadata → config → default
- Updated instruction generation to inject <context> and <rules> XML sections
- Integrated config creation prompts into `artifact-experimental-setup`

Schema resolution precedence:
1. --schema CLI flag (explicit override)
2. .openspec.yaml in change directory (change-specific)
3. openspec/config.yaml schema field (project default)
4. "spec-driven" (hardcoded fallback)

* test(config): add e2e tests and performance benchmarks for project config

- Add project config integration tests in artifact-workflow.test.ts:
  - Test new change uses schema from config
  - Test CLI schema overrides config schema
  - Test context and rules injection in instructions
  - Test backwards compatibility without config file
  - Test immediate reflection of config changes

- Add performance benchmark tests in project-config.test.ts:
  - Typical config (1KB): <20ms target
  - Large config (50KB): <50ms target
  - Repeated reads consistency check
  - Missing config fast-path test

- Add project configuration documentation in experimental-workflow.md:
  - Config fields reference (schema, context, rules)
  - Schema precedence explanation
  - Artifact IDs by schema
  - Troubleshooting guide

- Mark all tasks complete in tasks.md

* refactor(config): remove benchmark tests, document decision in source

Remove performance benchmark tests from project-config.test.ts and
document the results directly in the source code instead. Benchmarks
showed config reads are fast enough (~0.5ms typical) that caching
is unnecessary.

* fix(config): resolve three issues in project config feature

- Remove nested <template> tags: generateInstructions() no longer wraps
  template content since printInstructionsText() already handles XML
  structure
- Fix schema message accuracy: newChangeCommand() now uses the resolved
  schema returned from createChange() instead of hardcoded fallback
- Add TTY check: artifact-experimental-setup skips interactive prompts
  in non-TTY environments (CI, automation) to prevent hangs

* fix(config): handle null rules field in project config

Add null check when parsing rules field since YAML `rules:` with no
value parses to null, and `typeof null === 'object'` in JavaScript.
Without this check, Object.entries(null) would throw an error.
harikrishnan83 added a commit to intent-driven-dev/OpenSpec that referenced this pull request Jan 21, 2026
# By Tabish Bidiwale (57) and others
# Via GitHub
* main: (67 commits)
  fix(ci): use workflow_dispatch for polish release notes (Fission-AI#533)
  fix(changelog): convert markdown headers to bold text for proper formatting (Fission-AI#532)
  Version Packages (Fission-AI#531)
  Add changeset for project config and schema commands (Fission-AI#530)
  fix(config): handle null rules field in project config (Fission-AI#529)
  docs: update workflow docs and mark schema commands as experimental (Fission-AI#526)
  feat(cli): add schema management commands (Fission-AI#525)
  fix: Windows path compatibility in resolver tests (Fission-AI#524)
  change(schema-management-cli): proposal for schema management commands (Fission-AI#523)
  feat(resolver): add project-local schema support (Fission-AI#522)
  docs: add project-config demo guide (Fission-AI#521)
  feat(config): add project-level configuration via openspec/config.yaml (Fission-AI#499)
  fix: auto-trigger polish release notes on release publish (Fission-AI#519)
  perf: add path filtering to Nix validation CI job (Fission-AI#518)
  Version Packages (Fission-AI#517)
  Add changeset for v0.21 release (Fission-AI#516)
  fix: prevent implementation during explore mode (Fission-AI#515)
  OPSX apply: infer target change (Fission-AI#513)
  Refine opsx archive sync assessment (Fission-AI#514)
  feat: add nix flake support (sorry for this duplicate) (Fission-AI#459)
  ...

# Conflicts:
#	src/core/templates/slash-command-templates.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants