fix(core): fail loud on un-graduatable code-condition rule updates (#566)#587
Conversation
) Graduating an approved `update_rule` proposal for a CODE-condition rule (e.g. the procurement `manager_approval_threshold`, whose condition is a `CodeCondition` function) silently wrote a corrupt `defineRule({ "name": "…" })` stub — missing trigger/condition/effect — because `defaultCodegen`'s `?? { name: change.name }` fallback ran over a definition the NL resolver intentionally left undefined (`requiresCodeChange: true`), and `JSON.stringify` drops function-typed conditions. `ProposalFileWriter` now refuses such a change BEFORE any mkdir/writeFile: `assertGraduatable` (scoped to `rule`) throws when the effective definition lacks a required field (trigger/condition/effect) or carries a function where source needs data, naming the proposal id, change name, target and the fix (a `requiresCodeChange` update needs a materialized `generatedSource`). The trusted `generatedSource` path is untouched — materialized source still writes verbatim. Declarative rule updates with a full definition still graduate. Closes the silent-corruption half of #566. The AI materializer that regenerates the condition function (so "把阈值改成2万" graduates to valid code) is a tracked follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 19 minutes and 52 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)
📝 WalkthroughWalkthroughProposalFileWriter adds deterministic-serialization validation for rule changes via a new ChangesRule Graduatability Guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 introduces a graduatability guard (assertGraduatable) in ProposalFileWriter to prevent writing corrupt rule stubs when required fields (trigger, condition, effect) are missing or contain non-serializable functions. It also updates the test suite with helper functions and new test cases to validate this guard. The reviewer points out that running assertGraduatable unconditionally might break custom codegen implementations that are designed to handle these cases, and suggests restricting the guard to only run when the default codegen is used.
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.
Code ReviewReviewed No bugs found. The guard is correctly implemented:
One minor observation (not blocking): the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/__tests__/proposal-file-writer.test.ts (1)
1-885: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftFile exceeds 500-line guideline — split into focused modules.
This file is 885 lines, significantly exceeding the ~500-line guideline. Based on learnings, test files under
__tests__/should be split into multiple focused modules when approaching the limit (e.g., separate files for write behavior, filename format, formatter options, and engine hooks), with shared fixtures extracted to__tests__/helpers/*-fixtures.ts.The new
declarativeRuleChange()helper is a good start, but the file remains 77% over the limit.🤖 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/__tests__/proposal-file-writer.test.ts` around lines 1 - 885, The test file is too large; split it into focused test modules and extract shared fixtures/helpers: move makeApprovedProposal, declarativeRuleChange, viewChange, selfContainedEntityChange, FIXED_NOW/DATE_STAMP, TEST_* constants and todayUtcDateStamp into a new __tests__/helpers/proposal-fixtures.ts (or similar) and import them; then create separate test files for the main concerns — e.g. proposal-file-writer.writeApprovedProposal.test.ts (tests in the "ProposalFileWriter.writeApprovedProposal" describe), proposal-file-writer.filename.test.ts (the "filename format" describe), proposal-file-writer.formatter.test.ts (the "formatter option" describe), and proposal-engine.onApproved.test.ts (the "ProposalEngine.onApproved hook" describe) — each should import ProposalFileWriter, createProposalEngine, and the shared fixtures, keeping tests grouped by describe blocks like writeApprovedProposal, filename format, formatter option, and onApproved; ensure any per-test tmpDir setup/teardown (beforeEach/afterEach) is preserved or moved into a shared test helper to avoid duplication and update imports in all new files accordingly.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
packages/core/src/engine/proposal-file-writer.ts (1)
1-529: 💤 Low valueFile length exceeds 500-line guideline (529 lines).
The addition of
assertGraduatablepushed this file slightly over the limit. The current structure is cohesive—the guard is tightly coupled to the write path—so splitting may not be necessary immediately. Consider extracting the validation constants andassertGraduatablehelper to a separateproposal-validation.tsmodule if more guards are added in the future.🤖 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/proposal-file-writer.ts` around lines 1 - 529, File exceeds the 500-line guideline because assertGraduatable and its constants were added; extract the validation bits into a new module and import them to shrink the file: move RULE_REQUIRED_FIELDS and the function assertGraduatable (and any helpers they need like short error message fragments) into a new proposal-validation.ts module that exports them, update ProposalFileWriter to import { assertGraduatable, RULE_REQUIRED_FIELDS } from that module, keep the behaviour/signature identical, and run project tests/typechecks to ensure no breakage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/core/__tests__/proposal-file-writer.test.ts`:
- Around line 1-885: The test file is too large; split it into focused test
modules and extract shared fixtures/helpers: move makeApprovedProposal,
declarativeRuleChange, viewChange, selfContainedEntityChange,
FIXED_NOW/DATE_STAMP, TEST_* constants and todayUtcDateStamp into a new
__tests__/helpers/proposal-fixtures.ts (or similar) and import them; then create
separate test files for the main concerns — e.g.
proposal-file-writer.writeApprovedProposal.test.ts (tests in the
"ProposalFileWriter.writeApprovedProposal" describe),
proposal-file-writer.filename.test.ts (the "filename format" describe),
proposal-file-writer.formatter.test.ts (the "formatter option" describe), and
proposal-engine.onApproved.test.ts (the "ProposalEngine.onApproved hook"
describe) — each should import ProposalFileWriter, createProposalEngine, and the
shared fixtures, keeping tests grouped by describe blocks like
writeApprovedProposal, filename format, formatter option, and onApproved; ensure
any per-test tmpDir setup/teardown (beforeEach/afterEach) is preserved or moved
into a shared test helper to avoid duplication and update imports in all new
files accordingly.
---
Nitpick comments:
In `@packages/core/src/engine/proposal-file-writer.ts`:
- Around line 1-529: File exceeds the 500-line guideline because
assertGraduatable and its constants were added; extract the validation bits into
a new module and import them to shrink the file: move RULE_REQUIRED_FIELDS and
the function assertGraduatable (and any helpers they need like short error
message fragments) into a new proposal-validation.ts module that exports them,
update ProposalFileWriter to import { assertGraduatable, RULE_REQUIRED_FIELDS }
from that module, keep the behaviour/signature identical, and run project
tests/typechecks to ensure no breakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1af9cfc9-4d93-446c-9625-4d965460887f
📒 Files selected for processing (2)
packages/core/__tests__/proposal-file-writer.test.tspackages/core/src/engine/proposal-file-writer.ts
) A caller-supplied `codegen` (ProposalFileWriterOptions.codegen) is an escape hatch for unusual ChangeDefinition shapes and may legitimately handle a code-condition / definition-less rule update itself. `assertGraduatable` encodes the limits of `defaultCodegen` specifically (JSON.stringify drops functions; a rule needs trigger/condition/effect), so running it unconditionally would block custom generators. Gate the guard on `usesDefaultCodegen` (true only when no custom codegen was supplied). Adds a test: a custom codegen receives a definition-less code-condition rule update without the guard throwing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review summaryThe guard logic (
No other correctness or security issues found. The core fix — failing loud instead of writing a |
Code Review SummaryThe fix is correct and well-scoped. The guard runs before any mkdir/writeFile, so the corrupt-stub path is cleanly blocked with no partial I/O. The two detected cases (missing required field; function-valued field) are both tested and cover the real failure modes from #566. The escape hatch for custom codegen is a good design choice and is correctly tested. One issue filed inline: the diagnostic message in assertGraduatable hard-codes the word 'update' and would mislead a developer who hits the guard on a create operation. Everything else looks good: guard placement, generatedSource short-circuit, sequential writes, wx-flag atomicity for creates, and test coverage of all four paths. |
assertGraduatable fires on create AND update, but the hint string hardcoded "update" — misleading when a brand-new rule (operation: "create") carries a function condition. Use `change.operation` so the message reads "code-condition create needs…" / "code-condition update needs…" accurately. Adds a test pinning the create wording. (claude's other thread — guard pre-empting a custom codegen — was already fixed in e5c126f by scoping the guard to usesDefaultCodegen.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Code review — no bugs found. Reviewed proposal-file-writer.ts and the test suite. The guard is correct on all axes I checked: (1) RULE_REQUIRED_FIELDS including condition is consistent with validation-engine.ts which requires condition before a rule can be approved; (2) delete operations are skipped at line 432, before the assertGraduatable call, so the guard never fires on deletes; (3) the usesDefaultCodegen flag is safe — defaultCodegen is unexported so a caller cannot accidentally re-pass it and bypass the guard; (4) the guard fires before mkdir/writeFile so no partial state is left on disk on throw; (5) the upgraded declarativeRuleChange test helper correctly carries trigger/condition/effect and satisfies the guard. |
1 participant
What
Graduating an approved
update_ruleproposal for a code-condition rule silently wrote a corruptdefineRule({ "name": "…" })stub instead of valid source.ProposalFileWriternow fails loud before writing.Why (the silent-corruption hole, #566)
The procurement
manager_approval_thresholdrule has aCodeConditionfunction condition (threshold baked into code). When NL says "把经理审批阈值改成2万", the resolver (schema-intent-rule-updater) honestly refuses to fabricate a definition — it emits anupdatechange with nodefinitionandrequiresCodeChange: true.At graduation,
defaultCodegen's?? { name: change.name }fallback then serialized that to:— a stub missing
trigger/condition/effectthat fails validation once written.JSON.stringifyalso silently drops any function-typedcondition.The fix
assertGraduatable(proposal, change)(scoped torule) runs in the write path before anymkdir/writeFile, only when there is no trustedgeneratedSource:change.definition ?? {}) lackstrigger/condition/effect— therequiresCodeChange/ code-condition case.JSON.stringifywould drop it).Errors name the proposal id, change name, target, and the fix (a
requiresCodeChangeupdate needs a materializedgeneratedSource). The trustedgeneratedSourcepath is untouched (materialized source still writes verbatim); declarative rule updates with a full definition still graduate normally.Tests
generatedSource) → throws, no corrupt dir createdconditionis a function → throwsgeneratedSource→ writes verbatimdefineRule(...)(no regression)Several existing rule fixtures were upgraded from
{ name }-only stubs (themselves the corrupt shape the guard now rejects) to complete declarative definitions — behavior-preserving (path/filename/w-flag overwrite/updatedmarker all pinned). Fullpackages/coresuite: 4323 pass / 0 fail.Scope
Closes the silent-corruption half of #566. The AI materializer that regenerates the condition function (so "把阈值改成2万" graduates to valid code) is a tracked follow-up — not in this PR.
Part of #566.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests