Skip to content

fix: config set support object value - #1123

Closed
linjinze999 wants to merge 2 commits into
Fission-AI:mainfrom
linjinze999:feat/config_set_object
Closed

fix: config set support object value#1123
linjinze999 wants to merge 2 commits into
Fission-AI:mainfrom
linjinze999:feat/config_set_object

Conversation

@linjinze999

@linjinze999 linjinze999 commented May 25, 2026

Copy link
Copy Markdown

Summary by CodeRabbit

Release Notes

  • New Features

    • Added --json flag to config set command for parsing JSON input (arrays/objects).
    • Enhanced output formatting: strings now wrapped in quotes, objects/arrays JSON-stringified.
    • Prevents conflicting use of --string and --json flags simultaneously.
  • Tests

    • Added comprehensive test coverage for JSON configuration parsing and output formatting.

@linjinze999
linjinze999 requested a review from TabishB as a code owner May 25, 2026 06:47
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 33d7ce28-98b7-4fa3-bc1a-18a69c540ad3

📥 Commits

Reviewing files that changed from the base of the PR and between 58ff806 and 57d5e1f.

📒 Files selected for processing (3)
  • src/commands/config.ts
  • src/core/completions/command-registry.ts
  • test/commands/config-set-json.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/commands/config-set-json.test.ts

📝 Walkthrough

Walkthrough

This PR adds JSON input support to the openspec config set command. The handler parses --json values, validates flag conflicts and schema compliance, persists updated configuration, and formats output. The registry exposes the flag for completions. Comprehensive tests cover valid/invalid JSON, conflicts, validation failures, and output formatting.

Changes

Config JSON parsing feature

Layer / File(s) Summary
Config set handler with JSON parsing and validation
src/commands/config.ts
config set now accepts --json to parse JSON input, rejects --string+--json combinations, validates config schema before persisting, and formats output with proper quoting and JSON stringification for objects/arrays.
Command registry and completions
src/core/completions/command-registry.ts
Registers --json flag on config set command for CLI help and shell completions.
Test infrastructure and comprehensive test suite
test/commands/config-set-json.test.ts
Adds runConfigCommand test helper with isolated XDG storage and console mocking. Tests cover valid JSON arrays/objects, invalid/truncated JSON, flag conflicts, schema validation with regression check, output formatting consistency, and completion registry exposure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A rabbit hops with JSON in hand,
Config values now --json so grand!
Arrays and objects, no strings this time,
Parsed with care, in validation so fine,
Tests that cover each quirky crime! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: config set support object value' directly and clearly describes the main change: enabling the config set command to accept object (non-string) values, which aligns with the PR's core objective and the implementation shown in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@linjinze999

Copy link
Copy Markdown
Author

openspec config set workflows '["propose","explore","new","continue","apply","ff","sync","archive","bulk-archive","verify","onboard"]'

Error: Invalid configuration - workflows: Invalid input: expected array, received string

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right UX direction, but it needs the command metadata and tests before merge. config set --json is not added to src/core/completions/command-registry.ts, so completions/metadata will miss the new flag, and this config-writing path should have focused tests for JSON arrays/objects, invalid JSON, --string conflict, validation failure, output formatting, and no mutation on error.

@linjinze999
linjinze999 force-pushed the feat/config_set_object branch from c77a7a0 to 57d5e1f Compare June 4, 2026 13:07
@linjinze999

Copy link
Copy Markdown
Author

Ok! Addressed both in the latest push:

  • --json is now registered on config set in command-registry.ts, so completions/metadata pick it up.
  • Added test/commands/config-set-json.test.ts (16 tests) covering JSON arrays/objects, invalid JSON, --string/--json conflict, schema validation failures, output formatting, and verifying no on-disk mutation on any error path.

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up. The latest version adds the missing config-set metadata and focused regression coverage for arrays/objects, invalid JSON, --string conflicts, validation failures, output formatting, and no-write-on-error. I also re-ran the focused config test set locally: 95 tests passed. Looks good to me.

TabishB commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Closing this as superseded for now.

The original structured config value problem has landed in #1244, which auto-parses JSON-looking arrays/objects and fixes openspec config set workflows '[...]' on main. This PR still has a useful idea in the explicit config set --json UX and the cleaner JSON-style success output, but it is now behind main, only has an older CodeRabbit-only status signal, and would need to be reconciled with #1244's current behavior and tests.

If you want to continue this direction, please reopen or open a fresh small PR rebased on main that focuses only on the deliberate config set --json flag, its completion metadata, and updated post-#1244 tests. The main thing to define is how --json, automatic JSON-container parsing, and --string should interact.

Thanks for the careful test coverage here; it made the remaining value of the PR easy to understand.

@TabishB TabishB closed this Jun 24, 2026
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.

3 participants