fix(core): Phase-1 validation must accept sourcePatch rule updates (#566)#596
Conversation
) A "say→change an existing code-condition rule's threshold" proposal carries a definition-less change whose sourcePatch IS the spec. Phase-1 flagged it MISSING_DEFINITION, so the NL rule-threshold update could be RESOLVED but never APPROVED — the chain was silently broken at the approval gate (found by live testing the chat-assistant approve flow; #595's integration test bypassed approve by setting status directly). Skip the MISSING_DEFINITION requirement for a change carrying a sourcePatch, like revert/delete. The sourcePatch is value-validated at assembly (isSafeValueLiteral) and re-validated by the patcher at graduation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes Phase-1 proposal validation to prevent rejecting rule updates carrying a sourcePatch as MISSING_DEFINITION by skipping the definition requirement for these changes. The reviewer suggests adding basic static validation for the sourcePatch properties (such as filePath, constantName, and newValueLiteral) during Phase 1 to catch malformed proposals early, along with a corresponding test case to verify this validation.
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.
|
Warning Review limit reached
More reviews will be available in 46 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPhase-1 proposal validation now exempts changes carrying ChangessourcePatch Approval Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The MISSING_DEFINITION carve-out for sourcePatch changes skipped ALL Phase-1 validation, so a malformed sourcePatch (empty filePath/constantName/value) would fail opaquely at graduation. Now validate the patch's own shape early and emit INVALID_SOURCE_PATCH when incomplete (gemini). Adds a failing-case test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review summary — one finding. The fix correctly resolves the live-testing regression: Phase 1 was blocking every NL rule-threshold update with One narrowness issue flagged inline on line 205: the |
…Patch (#596 review) The carve-out's unconditional `continue` also skipped the target-specific definition `switch` for a change that carries BOTH a sourcePatch AND a definition (permitted by the type, not produced today) — silently accepting a malformed definition. Narrow it: still validate the sourcePatch shape, but only `continue` when there is NO definition; otherwise fall through to validateRule()/validateEntity()/… so an accompanying definition is still validated (claude). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`sourcePatch` is an optional field on the BASE ProposalChange, so the carve-out previously let a crafted entity/action change carrying a sourcePatch (no definition) bypass MISSING_DEFINITION and reach graduation with undefined behaviour. Narrow the guard to `target === "rule" && operation === "update"` — exactly the NL-threshold case it exists for; any other definition-less change (including a non-rule sourcePatch) now correctly fails MISSING_DEFINITION (claude). Adds a scope test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Reviewed. One real bug found (inline comment on line 207 of |
…as a re-flag of an already-fixed item)
| // through to MISSING_DEFINITION like any other definition-less change. | ||
| if (change.sourcePatch && change.target === "rule" && change.operation === "update") { | ||
| const { filePath, constantName, newValueLiteral } = change.sourcePatch; | ||
| if (!filePath || !constantName || !newValueLiteral) { |
There was a problem hiding this comment.
Bug: whitespace-only field values bypass the INVALID_SOURCE_PATCH guard
!filePath is false for " " (non-empty string), so a sourcePatch whose filePath/constantName/newValueLiteral contains only whitespace silently passes Phase 1 and is emitted as a well-formed change. It will then fail opaquely at graduation — exactly the "failing opaquely during graduation" scenario the comment on line 205 says this check was added to prevent.
| if (!filePath || !constantName || !newValueLiteral) { | |
| if (!filePath.trim() || !constantName.trim() || !newValueLiteral.trim()) { |
The same .trim() tightening should be applied in the test on line 366 (filePath: " ") to cover this path explicitly.
|
Overall the fix is correct and well-scoped. The Phase-1 carve-out is tightly constrained to One real bug filed inline: the |
1 participant
What — a real end-to-end gap, found by live testing
Phase-1 proposal validation rejected a
sourcePatch-carrying rule update asMISSING_DEFINITION, so a natural-language "say→change an existing code-condition rule's threshold" proposal could be resolved but never approved — the chain was silently broken at the approval gate.How it was found
Live-walking the freshly-landed chat-assistant channel (#594): typed "把经理审批阈值改成 20000" → the
SchemaProposalCardrendered correctly → clicked 批准 (Approve) →Proposal validation failed. Cannot approve.The approve envelope showed:A
sourcePatchchange deliberately has nodefinition— it rewrites a single named constant in existing source, so thesourcePatchIS its specification. Phase-1 didn't know that and blocked it. #595's integration test passed because it setstatus: "approved"directly, bypassing the approve-validation — so green tests never caught this.Fix
Phase-1 now skips the
MISSING_DEFINITIONrequirement for a change carrying asourcePatch, exactly like it already does forrevert/delete. Safety is unchanged: the value is validated at assembly (isSafeValueLiteral, #593) and re-validated by the patcher at graduation (NOT FOUND / AMBIGUOUS / NO INITIALIZER throw, #591).Tests
proposal-validation-phase1.test.tsgains a case asserting a definition-lesssourcePatchrule-update passes Phase 1 (mirroring the existingrevertcarve-out test). 149 validation-suite tests green;bun run check+bun run typecheckclean.This is the missing middle of #566 — the approve gate now accepts the governed sourcePatch draft, so resolve → approve → graduate completes end-to-end.
Related: #566, #591, #593, #595
Summary by CodeRabbit