feat: add package settings to manifest schema - #3
Conversation
📝 WalkthroughWalkthroughThis PR introduces a package settings feature to manifest v1 by defining a JSON Schema contract for user-configurable settings and documenting how they are persisted and used. Settings support boolean and enum types with required default values and must maintain stable identifiers across versions. ChangesPackage Settings Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
sabraman
changed the title
![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
🧹 Nitpick comments (1)
docs/package-contract.md (1)
81-85: ⚡ Quick winClarify that every setting declaration must include a
defaultvalue.The section describes types and stability, but not the requirement that each setting must provide
default. Adding that sentence here would keep author guidance aligned with validation behavior.Also applies to: 101-103
🤖 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 `@docs/package-contract.md` around lines 81 - 85, Add a single clear sentence to the "Supported setting types in manifest v1" section stating that every setting declaration must include a default value (e.g., "Each setting declaration must include a `default` value.") and duplicate the same sentence in the corresponding paragraph around the stability/types discussion later in the file (the similar block near lines 101-103) so author guidance matches validation behavior; reference the manifest version "manifest v1" and the term `default` to make locating and verifying the change straightforward.
🤖 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 `@schema/manifest.v1.schema.json`:
- Around line 102-103: Update the manifest schema so both setting variants
include "default" in their "required" arrays (i.e., add "default" alongside
"id","label","type") and enforce for enum settings that the declared default
appears in the "options" list; implement this either by adding a JSON Schema
constraint that validates the enum default against the "options" array or by
adding a check in tools/validate_manifest.py that verifies for each enum-type
setting the default value is present in its options array. Ensure you reference
the manifest schema's "required" array, the "default" property, and the enum
"options" property when making the changes.
---
Nitpick comments:
In `@docs/package-contract.md`:
- Around line 81-85: Add a single clear sentence to the "Supported setting types
in manifest v1" section stating that every setting declaration must include a
default value (e.g., "Each setting declaration must include a `default` value.")
and duplicate the same sentence in the corresponding paragraph around the
stability/types discussion later in the file (the similar block near lines
101-103) so author guidance matches validation behavior; reference the manifest
version "manifest v1" and the term `default` to make locating and verifying the
change straightforward.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e221bbc8-070a-4bda-a9f6-68e265b3e576
📒 Files selected for processing (2)
docs/package-contract.mdschema/manifest.v1.schema.json
| "required": ["id", "label", "type"], | ||
| "properties": { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the schema file
fd -t f "manifest.v1.schema.json" | head -1Repository: marginalia-os/marginalia-sdk
Length of output: 103
🏁 Script executed:
# Read the schema file to understand its structure and size
wc -l schema/manifest.v1.schema.jsonRepository: marginalia-os/marginalia-sdk
Length of output: 107
🏁 Script executed:
# Read the relevant section (lines 90-160 to get full context)
sed -n '90,160p' schema/manifest.v1.schema.json | cat -nRepository: marginalia-os/marginalia-sdk
Length of output: 2484
🏁 Script executed:
# Check if validate_manifest.py exists
fd -t f "validate_manifest.py"Repository: marginalia-os/marginalia-sdk
Length of output: 99
🏁 Script executed:
# Check the validate_manifest.py file size and read it
wc -l tools/validate_manifest.pyRepository: marginalia-os/marginalia-sdk
Length of output: 103
🏁 Script executed:
# Read the validation file to understand current enforcement
cat -n tools/validate_manifest.pyRepository: marginalia-os/marginalia-sdk
Length of output: 6029
Require setting defaults and validate enum defaults against declared options.
default is currently optional for both setting variants, and enum default is only constrained as a non-empty string. The validation tool performs only schema-level checks with no custom logic to enforce or verify these constraints. This allows manifests with missing or invalid initial values to pass validation.
Add default to the required array for both variants. For enum settings, validate that the default value is present in the options array (either through JSON Schema constraints or custom validation in tools/validate_manifest.py).
Suggested schema changes
{
"type": "object",
"additionalProperties": false,
- "required": ["id", "label", "type"],
+ "required": ["id", "label", "type", "default"],
"properties": {
"id": {
"type": "string",
"maxLength": 48,
"pattern": "^[a-zA-Z0-9][a-zA-Z0-9._-]*$" {
"type": "object",
"additionalProperties": false,
- "required": ["id", "label", "type", "options"],
+ "required": ["id", "label", "type", "options", "default"],
"properties": {
"id": {
"type": "string",
"maxLength": 48,
"pattern": "^[a-zA-Z0-9][a-zA-Z0-9._-]*$"🤖 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 `@schema/manifest.v1.schema.json` around lines 102 - 103, Update the manifest
schema so both setting variants include "default" in their "required" arrays
(i.e., add "default" alongside "id","label","type") and enforce for enum
settings that the declared default appears in the "options" list; implement this
either by adding a JSON Schema constraint that validates the enum default
against the "options" array or by adding a check in tools/validate_manifest.py
that verifies for each enum-type setting the default value is present in its
options array. Ensure you reference the manifest schema's "required" array, the
"default" property, and the enum "options" property when making the changes.
1 participant
Summary
Adds manifest v1 schema support for package settings. The schema now accepts boolean and enum setting declarations, and the package contract documents how firmware persists those values outside package archives.
Validation
Summary by CodeRabbit
New Features
Documentation