feat(core,adapter): assemble sourcePatch for say→change-rule (#566)#593
Conversation
…hange-rule (#566) When an utterance asks to change a constant a code-condition rule owns ("raise the manager-approval threshold to 20000"), the resolver emits a strictly validated newValueLiteral and draftRuleUpdate assembles a ProposalDiff.sourcePatch {filePath, constantName, newValueLiteral} when the rule opts in via the new RuleDefinition.patchTarget. The adapter-server route copies it onto the governed ProposalChange.sourcePatch consumed by ProposalFileWriter; both adapter-server and adapter-mcp project patchTarget into the AI rule snapshot. Security: isSafeValueLiteral admits only number / boolean / null / JSON-string literals (no expressions, identifiers, or multi-token input), re-validated at assembly time. The demo manager-approval-threshold rule opts in. Runtime injection of the real TS-AST patcher is a separate capstone PR; tests here prove assembly + graduation with an injected fake patcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 36 minutes and 1 second. 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 (14)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request implements the natural-language path to change an existing code-condition rule's threshold via a structured sourcePatch (#566). It introduces isSafeValueLiteral to validate the new value literal, adds patchTarget to RuleDefinition to opt into this behavior, and updates the adapter-server and adapter-mcp to propagate these patches to the ProposalFileWriter. Feedback on the changes suggests refining the regular expression in isSafeValueLiteral to disallow number literals with leading zeros (e.g., 0123), which would otherwise trigger strict-mode compile-time syntax errors regarding octal literals.
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.
…593 review) A leading-zero integer (`007`, `0123`) splices into source as an octal literal — a syntax error in strict-mode TypeScript. Tighten the number regex to disallow leading zeros on non-zero integers (gemini), and add 007/0123/00 regression cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| constantName: string; | ||
| /** Already-validated literal to write as the new value, e.g. "20000". */ | ||
| newValueLiteral: string; | ||
| }; |
There was a problem hiding this comment.
Type-drift risk: SchemaIntentProposalDraft.sourcePatch (this block) and ProposalDiff["sourcePatch"] in proposal-engine.ts declare the same three-field shape independently. Because TypeScript structural typing allows assignability in either direction, if ProposalDiff["sourcePatch"] gains or drops a required field the assignment in draftRuleUpdate will still compile silently — the new field would disappear from the outcome object without a compile error at the declaration site.
buildSourcePatch already anchors its return type as NonNullable<ProposalDiff["sourcePatch"]>, so the fix is to derive this field's type from that same anchor:
| }; | |
| }; |
sourcePatch?: NonNullable<ProposalDiff["sourcePatch"]>;(Add import type { ProposalDiff } from "./proposal-engine"; at the top of this file.) Future shape changes to ProposalDiff["sourcePatch"] would then propagate automatically.
| effectType: "warn", | ||
| conditionKind: "declarative", | ||
| condition: { field: "amount", operator: "gt", value: 5000 }, | ||
| roundTrippable: true, |
There was a problem hiding this comment.
Test doesn't exercise buildSourcePatch at all. The title says "builds NO sourcePatch for a declarative (non-code) rule", but roundTrippable: true means the outer condition in draftRuleUpdate
if (existing.conditionKind === "code" || existing.roundTrippable === false)is false, so execution takes the declarative rebuild path and buildSourcePatch is never called. The test therefore provides zero coverage for the guard inside buildSourcePatch:
if (existing.conditionKind !== "code") return undefined;Mutation: changing that guard to conditionKind !== "declarative" would pass every existing test while silently allowing a declarative-rule diff-only path to emit a sourcePatch.
The missing case is a rule where both the outer condition and buildSourcePatch's guard are relevant:
// roundTrippable: false → enters the diff-only branch in draftRuleUpdate
// conditionKind: "declarative" → buildSourcePatch must return undefined despite having a patchTarget + safe literal
const nonRoundTrippableDeclarative: SchemaIntentRule = {
...codeRuleWithPatchTarget({ conditionKind: "declarative", roundTrippable: false }),
};
const outcome = runDraft({
parsed: parsedUpdate({ newValueLiteral: "20000" }), // no rule: field → noMatch guard
rule: nonRoundTrippableDeclarative,
});
// Diff-only path, buildSourcePatch called but conditionKind !== "code" → sourcePatch absent
expect(outcome.sourcePatch).toBeUndefined();This test would fail under the above mutation, providing the missing coverage.
|
Code review summary — two inline findings posted. The core security design (isSafeValueLiteral + defence-in-depth re-validation in buildSourcePatch + path-traversal check in applySourcePatch) is solid, and the validator test battery is thorough.
|
…duates to real source (#566 capstone) (#595) * feat(adapter-server): inject patchNamedConstant — say→change-rule graduates to real source (#566 capstone) The proposal graduate API gains an injectable `sourcePatcher` option, threaded into its DEFAULT writer; server.ts (the composition root) wires in `patchNamedConstant` from @linchkit/devtools — the TS-AST patcher behind core's typescript-free SourcePatcher seam. An approved code-condition rule update carrying a `sourcePatch` now rewrites the named constant in REAL source during graduation, then opens the usual human-reviewed PR. This closes the natural-language → real-code arm: with the resolver assembly (#593) + the patcher (#591), "raise the manager-approval threshold to 20000" now graduates by editing `export const MANAGER_APPROVAL_THRESHOLD`. Adds @linchkit/devtools as a dependency (capability seam: core stays typescript-free; the heavy patcher lives in devtools, injected here). New integration test drives the full route → default writer → real file patch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(adapter-server): assert status before parsing body (#595 review) Move the `res.status` assertion ahead of `res.json()` so a non-JSON error body surfaces as a clear status-code mismatch instead of a confusing SyntaxError (gemini). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Wires the natural-language "say→change an existing code-condition rule's threshold" path to a structured
sourcePatch(#566). This is the core assembly brick that turns an utterance like "把经理审批阈值改成 20000" into a governed Proposal whose change carries the exact{ filePath, constantName, newValueLiteral }theProposalFileWriter's injectedSourcePatcher(#590 / #591) needs to rewrite the realexport const MANAGER_APPROVAL_THRESHOLDin source.How
schema-intent-prompt.ts—ParsedSchemaIntent.newValueLiteral, the prompt instruction to emit it, and the strictisSafeValueLiteralvalidator (number / boolean / null / double-quoted JSON string only — no expressions, identifiers,Infinity/NaN, multi-token, or whitespace). Unsafe values are dropped at parse time.types/rule.ts— opt-inRuleDefinition.patchTarget { sourcePath, constantName }. A code-condition rule declares the constant it owns; without it, no sourcePatch is built (existing fail-loud guard from fix(core): fail loud on un-graduatable code-condition rule updates (#566) #587 still applies).schema-intent-rule-updater.ts—draftRuleUpdateassemblessourcePatchonly whenconditionKind === "code"+patchTargetpresent +newValueLiteralsafe (re-validated as defence-in-depth).proposal-engine.ts/schema-intent-types.ts—ProposalDiff.sourcePatch+SchemaIntentRule.patchTargetso theaisingle-change model can carry it.toSchemaIntentRule()projectspatchTarget; the adapter-serverresolve-schema-intentroute copiesoutcome.sourcePatchonto the governedProposalChange.sourcePatch.Scope
Runtime injection of the real TS-AST patcher (
patchNamedConstantfrom@linchkit/devtools, merged in #591) is a separate capstone PR —proposal-graduate-api.tsis intentionally untouched here. The e2e graduation test proves assembly→patch with an injected fake patcher.Tests
New
schema-intent-value-literal.test.ts(validator + parse gating, incl. malicious inputs) andschema-intent-rule-updater.test.ts(safe→patch; absent / unsafe / no-patchTarget / declarative → no patch), plus an extendedproposal-file-writer.test.tsassembly→graduation loop.bun run check+bun run typecheckclean; targeted core/adapter/demo suites green.Related: #566, #587, #590, #591