feat(config): define versioned compatibility contracts - #73
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughConfiguration format v2 is introduced with strict versioned types and validation, a dry-run-first ChangesConfiguration v2 contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant MigrationPlanner
participant MigrationPublisher
participant ConfigFile
Operator->>CLI: run migrate-config
CLI->>MigrationPlanner: validate and plan v1 to v2 migration
MigrationPlanner-->>CLI: migration report
Operator->>CLI: rerun with --write
CLI->>MigrationPublisher: apply validated migration
MigrationPublisher->>ConfigFile: create exclusive backup and publish candidate
ConfigFile-->>CLI: updated configuration and backup status
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/cli/migrate-config.ts`:
- Around line 327-434: Extract the repeated restore-and-escalate logic from
installWithoutOverwriting into a small helper that performs the appropriate
restore operation, removes the candidate and transaction when restoration
succeeds, and throws the supplied safe Migration*Error; otherwise throw
MigrationTransactionError. Replace each applicable restoreHeldSource and
restoreCurrentHeldSource branch while preserving their distinct restoration
arguments and error outcomes.
In `@src/config/schema.ts`:
- Around line 8-19: Update the error message in configVersionSchema to derive
the supported version list from SUPPORTED_CONFIG_VERSIONS instead of hardcoding
'1' and '2'. Preserve the existing unsupported-version code, remediation, and
validation behavior while formatting the array values consistently in the
message.
- Around line 1216-1229: Update the alias checks for security.redactSecrets and
audit.redact in the shown schema validation flow to run only when their values
are defined and not false. Preserve the existing alias rejection for non-false
values, allowing the later === false validation to produce the sole diagnostic
for explicit false settings.
In `@tests/windows-config-migration-acl.test.ts`:
- Around line 250-275: Update windowsAclSddl to apply the same 5-second timeout
and child.kill cleanup pattern used by windowsPrivateDirectoryProbe and
windowsCopyFileSecurityProbe. Ensure the timeout rejects the promise with the
established probe failure error, and clear the timer when the child exits or
emits an error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f9fce09-0433-4707-91fe-15a346fcbaac
📒 Files selected for processing (50)
CHANGELOG.mdREADME.mddocs/architecture.mddocs/cli.mddocs/config.mddocs/library-api.mddocs/plugins.mdexamples/generic.miftah.jsonexamples/github.miftah.jsonexamples/multi-upstream.miftah.jsonexamples/plugins.miftah.jsonexamples/sentry.miftah.jsonpackage.jsonsrc/audit/audit-logger.tssrc/audit/audit-types.tssrc/cli/exit-codes.tssrc/cli/main.tssrc/cli/migrate-config.tssrc/cli/parse.tssrc/cli/windows-config-acl.tssrc/config/generate-json-schema.tssrc/config/load-config.tssrc/config/migrate-config.tssrc/config/presets.tssrc/config/schema.tssrc/config/types.tssrc/config/versions.tssrc/index.tssrc/plugins/plugin-registry.tssrc/runtime/resolve-runtime-config.tssrc/utils/errors.tstests/audit-integrity.test.tstests/audit.test.tstests/cli-exit-codes.test.tstests/cli-parse.test.tstests/config-diagnostics.test.tstests/config-loader.test.tstests/config-migration.test.tstests/config-schema-contract.test.tstests/fixtures/config-compat/v1/generic-0.1.0.miftah.jsontests/fixtures/config-compat/v2/canonical.miftah.jsontests/mcp-wrapper.test.tstests/package-contract.test.tstests/plugin-api.test.tstests/public-api.test.tstests/release-config.test.tstests/runtime-config-resolution.test.tstests/windows-config-acl.test.tstests/windows-config-migration-acl-failure.test.tstests/windows-config-migration-acl.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/windows-config-acl.ts`:
- Around line 27-28: Update the ACL persistence verification in the relevant
flow around the selected-section reread to compare the target DiscretionaryAcl
or equivalent SDDL against the source ACL before returning. Retain the existing
reread and failure handling, but reject persistence when the target DACL does
not match the source rather than only checking that an access-control object was
returned.
In `@tests/windows-config-acl.test.ts`:
- Around line 57-73: Update the test for copyWindowsConfigSecurityDescriptor to
reflect strengthened target verification: replace the assertion that the
generated command omits $verifiedAcl.GetSecurityDescriptorBinaryForm with
assertions confirming it validates $verifiedAcl.DiscretionaryAcl, while
preserving the existing source ACL and binary descriptor assertions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10d814fd-2400-429c-a549-020ab9b9013d
📒 Files selected for processing (3)
src/cli/windows-config-acl.tstests/windows-config-acl.test.tstests/windows-config-migration-acl.test.ts
1 participant
Summary
Compatibility
MiftahConfigis intentionally version-discriminated. This is an explicit pre-1.0 breaking type-surface change documented in the changelog and library API documentation; consumers composing application config should use an intersection type. Please review and approve this migration before merge.Validation
npm test— 991 passed, 14 skippednpm run test:core— 387 passed, 14 skippednpm run test:coverage— 95.46% statements overall; config 95.76%npm run lintnpm run typechecknpm run buildnpm run smoke:clinpm run check:packnpm run test:package— 16 passedCloses #38
Summary by CodeRabbit
miftah migrate-configwith dry-run reporting, plus safe--writebehavior using exact backups.miftah_list_approvals,miftah_approve,miftah_deny.httptransport) and v2 runtime exports are now explicitly documented.