feat(core): implement proposal-validation Phase 3 compatibility checks (Spec 09 §4.5)#487
Conversation
📝 WalkthroughWalkthroughThis pull request introduces Phase 3 proposal validation for detecting breaking references against the current meta-model. It detects deleted fields referenced by views/rules, deleted actions/states with dependents, and narrowing changes (type changes, enum removal, required-default drops). Findings are warn-only by default and become blocking when ChangesPhase 3 Validation Feature
🎯 3 (Moderate) | ⏱️ ~28 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)
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 |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 3 of proposal validation, which checks for breaking compatibility and reference changes (such as deleting referenced fields, actions, or states, changing field types, or narrowing enum values) against the current meta-model. These checks are warn-only by default but can escalate to blocking errors in production environments via the strictCompatibility flag. The review feedback highlights several critical robustness improvements in validation-phase3.ts, including potential runtime crashes when handling optional view.fields or nullish rule triggers, and a functional gap where string-based enum options are not correctly mapped.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/engine/validation-engine.ts (1)
642-646:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOutdated docstring — Phase 3 now runs.
The docstring still says "Currently only runs Phase 1 (static checks)" but Phase 3 is now wired in. Update the comment to reflect this.
📝 Suggested fix
/** - * Validate a full proposal. Currently only runs Phase 1 (static checks). - * Phase 2-4 will be added in later milestones. + * Validate a full proposal. Runs Phase 1 (static checks) and Phase 3 + * (compatibility / breaking-reference checks). Phase 2/4 are skipped for M1. */🤖 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 `@packages/core/src/engine/validation-engine.ts` around lines 642 - 646, Update the docstring that currently reads "Validate a full proposal. Currently only runs Phase 1 (static checks)." (the comment above the full-proposal validation function) to accurately state that Phase 3 is now executed as well — e.g., "Validate a full proposal. Runs Phase 1 (static checks) and Phase 3 (runtime checks); Phases 2 and 4 are planned for later." Keep the descriptive text near the same comment block so it clearly documents the behavior of the full-proposal validation routine.
🤖 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 @.changeset/validation-phase3-compatibility.md:
- Line 8: Update the changeset sentence that currently states "dev/staging stays
advisory" to correctly reflect the actual wiring of features.strictCompatibility
(which sets strictCompatibility = true for production and staging, false for
dev/test); reword it to indicate that staging is blocking like production while
dev remains advisory, and ensure the text mentions mountProposalAPI and
ValidationContext.strictCompatibility behavior so the release note matches
runtime behavior.
---
Outside diff comments:
In `@packages/core/src/engine/validation-engine.ts`:
- Around line 642-646: Update the docstring that currently reads "Validate a
full proposal. Currently only runs Phase 1 (static checks)." (the comment above
the full-proposal validation function) to accurately state that Phase 3 is now
executed as well — e.g., "Validate a full proposal. Runs Phase 1 (static checks)
and Phase 3 (runtime checks); Phases 2 and 4 are planned for later." Keep the
descriptive text near the same comment block so it clearly documents the
behavior of the full-proposal validation routine.
🪄 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: d4fbee8b-5bc4-4f80-9b38-a17f9d7d383e
📒 Files selected for processing (10)
.changeset/validation-phase3-compatibility.mdaddons/adapter-server/cap-adapter-server/src/proposal-api.tsaddons/adapter-server/cap-adapter-server/src/server.tspackages/core/__tests__/barrel-public-surface.test.tspackages/core/src/__tests__/engine/validation-phase3.test.tspackages/core/src/deployment/environment.tspackages/core/src/engine/index.tspackages/core/src/engine/validation-engine.tspackages/core/src/engine/validation-phase3.tspackages/core/src/exports/server/engines.ts
…s (Spec 09 §4.5) Proposal validation only ran Phase 1 (static checks); Phase 2/3/4 were hardcoded "skipped" stubs, so a proposal that removes a field an action/view/ rule depends on passed validation. This makes the "AI never modifies production directly" guardrail shallow. Implement Phase 3 (compatibility / breaking-reference detection): - New `packages/core/src/engine/validation-phase3.ts` exporting `validatePhase3`. It inspects delete/narrowing changes against the current meta-model via the OntologyRegistry impact graph (action/state deletes) + a field-reference scan (entity-field deletes referenced by views/rules) + field-shape comparison (type change, required-default drop, enum-value removal). - Wire it into `validateProposal` (replace the phase3 skipped stub). - Extend `ValidationContext` with optional `ontology` + `strictCompatibility`. Gating (low-regret, fail-closed per Spec 09 §4.1): - DEFAULT warn-only — findings are warnings, `passed` is NOT affected. - `strictCompatibility` escalates findings to blocking errors. New `features.strictCompatibility` env flag (true in prod, false in dev/test, sibling of `strictValidation`) threads through `mountProposalAPI` so PRODUCTION refuses breaking proposals while dev/staging stays advisory. Backward compatible: when the context carries no ontology, Phase 3 returns "skipped" — identical to the prior stub. The detector is conservative (deterministic reference checks; skips opaque code conditions and cross-entity refs it cannot resolve), so it under-reports rather than false-positive-blocks. Stays strictly within the validation boundary — no approval/commit/deploy/ graduation/file-write path is touched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b69a41d to
1cedd34
Compare
What & why
Proposal validation (
validateProposal) only ran Phase 1 (static checks) — Phase 2/3/4 were hardcodedstatus: "skipped"stubs. So a proposal that removes a field an action/view/rule depends on passed validation. This makes the "AI never modifies production directly" guardrail shallow (G3).Change
Implement Phase 3 (compatibility / breaking-reference detection), Spec 09 §4.5:
packages/core/src/engine/validation-phase3.ts(validatePhase3). Detects, against the current meta-model:BREAKING_FIELD_DELETEBREAKING_ELEMENT_DELETEBREAKING_FIELD_TYPE_CHANGEBREAKING_REQUIRED_DEFAULT_DROPBREAKING_ENUM_VALUE_REMOVEDvalidateProposal(replaces the phase3 stub).ValidationContextgains optionalontology+strictCompatibility.Gating (fail-closed per Spec 09 §4.1) —⚠️ changes production behavior
passedis NOT affected.strictCompatibilityescalates to blocking errors. Newfeatures.strictCompatibilityenv flag — true in production and staging, false in dev/test (sibling ofstrictValidation; noteisProductioncovers both production and staging) — threaded throughmountProposalAPI. So production and staging now refuse proposals that break existing references; dev/test stays advisory. Flagged here explicitly because it changes pre-prod/prod validation behavior. The detector is deterministic and conservative (reference checks via the impact graph; skips opaque code conditions and refs it cannot resolve) → it under-reports rather than false-positive-blocks.Backward compatible
When the context carries no
ontology, Phase 3 returns"skipped"— identical to the prior stub. Existing validation tests pass untouched.Known limitation (documented, follow-up)
The impact DAG does not yet model
StateDefinition.transitions[].action/ entity state-fieldmachineedges, so deleting an action used only as a state transition may not surface a dependent (under-report). Noted in code; tracked for a future ontology-DAG enhancement.Review
Cross-model reviewed (codex + PR bots gemini-code-assist & CodeRabbit). All findings addressed:
view.fields?.andrule?.triggerguards added; the "enum options can be strings" note was verified non-applicable (EnumField.optionsis typedArray<{value,label?}>).Tests
validation-phase3.test.ts— 18 tests (unit +validateProposalintegration): degradation/skip, field-delete warn vs strict-block, whole-entity delete, action/state delete, type/enum/default narrowing, required→optional loosening (no false positive), and the integration warn-vs-block path.Gates
bun run check,bun run typecheck, fullbun run test(batched) all green. Stays strictly within the validation boundary — no approval/commit/deploy/graduation/file-write path touched.🤖 Generated with Claude Code