fix(core): fail loud on un-graduatable code-condition rule updates (#566)

packages/core/__tests__/proposal-file-writer.test.ts

@@ -46,10 +46,16 @@ function makeApprovedProposal(overrides: Partial<ProposalDefinition> = {}): Prop
     target: "rule",
     operation: "create",
     name: "auto_approve_small_orders",
+    // A complete DECLARATIVE rule definition (trigger + data condition + effect).
+    // The writer's graduatability guard refuses a rule definition missing any of
+    // those three (it would otherwise graduate as a corrupt `defineRule({name})`
+    // stub — #566), so every rule fixture carries a faithfully-serialisable shape.
     definition: {
       name: "auto_approve_small_orders",
-      trigger: { type: "manual" },
-      effect: { type: "set_field", field: "status", value: "approved" },
+      label: "Auto-approve small orders",
+      trigger: { action: "submit_order" },
+      condition: { field: "amount", operator: "lt", value: 100 },
+      effect: { type: "enrich", setFields: { status: "approved" } },
     } as never, // RuleDefinition shape varies; cast to keep test fixture compact.
     diff: "Auto-approve orders under $100.",
   };
@@ -78,6 +84,30 @@ function makeApprovedProposal(overrides: Partial<ProposalDefinition> = {}): Prop
   };
 }
 
+/**
+ * A complete declarative rule change — trigger + data condition + effect — so
+ * the writer's graduatability guard (#566) accepts it. Use this anywhere a test
+ * needs a real, faithfully-serialisable rule (not the code-condition draft that
+ * the guard intentionally refuses).
+ */
+function declarativeRuleChange(
+  name: string,
+  operation: ProposalChange["operation"] = "create",
+): ProposalChange {
+  return {
+    target: "rule",
+    operation,
+    name,
+    definition: {
+      name,
+      label: name,
+      trigger: { action: "submit_order" },
+      condition: { field: "amount", operator: "lt", value: 100 },
+      effect: { type: "enrich", setFields: { status: "approved" } },
+    } as never,
+  };
+}
+
 function viewChange(name = "order_kanban"): ProposalChange {
   return {
     target: "view",
@@ -234,12 +264,7 @@ describe("ProposalFileWriter.writeApprovedProposal", () => {
     const proposal = makeApprovedProposal({
       changes: [
         // First change: a rule
-        {
-          target: "rule",
-          operation: "create",
-          name: "auto_approve_small_orders",
-          definition: { name: "auto_approve_small_orders" } as never,
-        },
+        declarativeRuleChange("auto_approve_small_orders"),
         // Second change: a view
         viewChange(),
       ],
@@ -258,20 +283,7 @@ describe("ProposalFileWriter.writeApprovedProposal", () => {
   it("writes two changes of the same target kind to distinct files (no collision)", async () => {
     const writer = new ProposalFileWriter({ rootDir: tmpDir });
     const proposal = makeApprovedProposal({
-      changes: [
-        {
-          target: "rule",
-          operation: "create",
-          name: "rule_one",
-          definition: { name: "rule_one" } as never,
-        },
-        {
-          target: "rule",
-          operation: "create",
-          name: "rule_two",
-          definition: { name: "rule_two" } as never,
-        },
-      ],
+      changes: [declarativeRuleChange("rule_one"), declarativeRuleChange("rule_two")],
     });
 
     const written = await writer.writeApprovedProposal(proposal);
@@ -320,7 +332,17 @@ describe("ProposalFileWriter.writeApprovedProposal", () => {
           target: "rule",
           operation: "update",
           name: "auto_approve_small_orders",
-          definition: { name: "auto_approve_small_orders", updated: true } as never,
+          // A full declarative update (trigger + data condition + effect) — the
+          // graduatability guard accepts it and the `w` flag overwrites the stale
+          // file. `updated` is an extra marker the assertion below pins on.
+          definition: {
+            name: "auto_approve_small_orders",
+            label: "Auto-approve small orders",
+            trigger: { action: "submit_order" },
+            condition: { field: "amount", operator: "lt", value: 100 },
+            effect: { type: "enrich", setFields: { status: "approved" } },
+            updated: true,
+          } as never,
         },
       ],
     });
@@ -347,6 +369,160 @@ describe("ProposalFileWriter.writeApprovedProposal", () => {
     expect(contents).toContain('"updated": true');
   });
 
+  // ── Graduatability guard (#566 — silent-corruption hole) ──────────────────
+  //
+  // A code-condition rule update is the canonical un-serialisable case: the NL
+  // resolver (schema-intent-rule-updater) honestly refuses to fabricate a
+  // definition (`requiresCodeChange: true`, no `definition`). Without the guard,
+  // the writer's `?? { name: change.name }` fallback graduated a corrupt
+  // `defineRule({ "name": "…" })` stub — missing trigger/condition/effect. The
+  // guard now FAILS LOUD instead of writing that stub. The full AI materializer
+  // (which regenerates the condition function → `generatedSource`) is a separate
+  // follow-up.
+
+  it("THROWS on a code-condition rule update with no definition and no generatedSource (#566)", async () => {
+    const writer = new ProposalFileWriter({ rootDir: tmpDir });
+    // The honest code-condition draft: an `update` with NO definition (the
+    // resolver intentionally left it undefined) and NO materialized source.
+    const proposal = makeApprovedProposal({
+      changes: [
+        {
+          target: "rule",
+          operation: "update",
+          name: "manager_approval_threshold",
+          diff: "把经理审批阈值改成 2 万",
+          // No `definition`, no `generatedSource`.
+        },
+      ],
+    });
+
+    await expect(writer.writeApprovedProposal(proposal)).rejects.toThrow(
+      /manager_approval_threshold/,
+    );
+    await expect(writer.writeApprovedProposal(proposal)).rejects.toThrow(
+      /materialization|generatedSource/i,
+    );
+
+    // No corrupt stub was written.
+    const rulesDir = join(tmpDir, "addons", "demo", "cap-life-demo", "src", "rules");
+    expect(existsSync(rulesDir)).toBe(false);
+  });
+
+  it("THROWS on a rule update whose condition is a function (JSON.stringify would drop it) (#566)", async () => {
+    const writer = new ProposalFileWriter({ rootDir: tmpDir });
+    // A definition that LOOKS complete but carries a function condition — the
+    // deterministic codegen (`JSON.stringify`) would silently drop it, emitting
+    // a rule with `condition` lost. The guard refuses rather than corrupt.
+    const proposal = makeApprovedProposal({
+      changes: [
+        {
+          target: "rule",
+          operation: "update",
+          name: "manager_approval_threshold",
+          definition: {
+            name: "manager_approval_threshold",
+            label: "Manager approval threshold",
+            trigger: { action: "submit_request" },
+            // Code condition — a real function, not declarative data.
+            condition: (ctx: { target: Record<string, unknown> }) =>
+              Number(ctx.target.amount) > 10000,
+            effect: { type: "require_approval", level: "manager" },
+          } as never,
+        },
+      ],
+    });
+
+    await expect(writer.writeApprovedProposal(proposal)).rejects.toThrow(/function/i);
+    await expect(writer.writeApprovedProposal(proposal)).rejects.toThrow(
+      /manager_approval_threshold/,
+    );
+
+    // Nothing written.
+    const rulesDir = join(tmpDir, "addons", "demo", "cap-life-demo", "src", "rules");
+    expect(existsSync(rulesDir)).toBe(false);
+  });
+
+  it("writes generatedSource verbatim for a code-condition rule update (guard skipped) (#566)", async () => {
+    const writer = new ProposalFileWriter({ rootDir: tmpDir });
+    const GENERATED = [
+      'import { defineRule } from "@linchkit/core";',
+      "export default defineRule({",
+      '  name: "manager_approval_threshold",',
+      '  label: "Manager approval threshold",',
+      '  trigger: { action: "submit_request" },',
+      "  condition: (ctx) => Number(ctx.target.amount) > 20000,",
+      '  effect: { type: "require_approval", level: "manager" },',
+      "});",
+      "",
+    ].join("\n");
+    const proposal = makeApprovedProposal({
+      changes: [
+        {
+          target: "rule",
+          operation: "update",
+          name: "manager_approval_threshold",
+          // Materialized source present → trusted, written verbatim, no guard.
+          generatedSource: GENERATED,
+        },
+      ],
+    });
+
+    const [written] = await writer.writeApprovedProposal(proposal);
+    expect(written).toBeDefined();
+    const contents = await readFile(written as string, "utf8");
+    expect(contents).toBe(GENERATED);
+    expect(contents).toContain("condition: (ctx) => Number(ctx.target.amount) > 20000");
+  });
+
+  it("writes a valid defineRule file for a full declarative rule update (no regression) (#566)", async () => {
+    const writer = new ProposalFileWriter({ rootDir: tmpDir });
+    const proposal = makeApprovedProposal({
+      changes: [declarativeRuleChange("auto_approve_small_orders", "update")],
+    });
+
+    const [written] = await writer.writeApprovedProposal(proposal);
+    const contents = await readFile(written as string, "utf8");
+    // Valid defineRule(...) source with the declarative condition serialised.
+    expect(contents).toContain("export default defineRule(");
+    expect(contents).toContain('"trigger"');
+    expect(contents).toContain('"condition"');
+    expect(contents).toContain('"effect"');
+    expect(contents).toContain('"operator": "lt"');
+  });
+
+  it("does NOT apply the guard when a custom codegen is supplied (escape hatch) (#566)", async () => {
+    // A caller-supplied `codegen` is the escape hatch for unusual ChangeDefinition
+    // shapes — it may legitimately handle a code-condition / definition-less rule
+    // update itself. The graduatability guard encodes `defaultCodegen`'s limits
+    // only, so it must NOT pre-empt a custom generator (gemini review on #587).
+    const CUSTOM = "// custom generator output\nexport default {/* hand-rolled */};\n";
+    let sawChange = false;
+    const writer = new ProposalFileWriter({
+      rootDir: tmpDir,
+      codegen: () => {
+        sawChange = true;
+        return CUSTOM;
+      },
+    });
+    const proposal = makeApprovedProposal({
+      changes: [
+        {
+          target: "rule",
+          operation: "update",
+          name: "manager_approval_threshold",
+          // The shape the default guard rejects (no definition, no generatedSource)
+          // — but the custom codegen owns it, so no throw.
+          diff: "把经理审批阈值改成 2 万",
+        },
+      ],
+    });
+
+    const [written] = await writer.writeApprovedProposal(proposal);
+    expect(sawChange).toBe(true);
+    const contents = await readFile(written as string, "utf8");
+    expect(contents).toBe(CUSTOM);
+  });
+
   it("throws when proposal is not approved", async () => {
     const writer = new ProposalFileWriter({ rootDir: tmpDir });
     const draft = makeApprovedProposal({ status: "draft" });

packages/core/src/engine/proposal-file-writer.ts

@@ -238,9 +238,72 @@ function assertWritableTarget(target: ProposalChangeTarget): WritableChangeTarge
   return target;
 }
 
+/** Fields a `rule` definition must carry to be deterministically serialisable. */
+const RULE_REQUIRED_FIELDS = ["trigger", "condition", "effect"] as const;
+
+/**
+ * Refuse to deterministically serialise a change whose effective definition
+ * cannot round-trip to valid source — FAIL LOUD instead of writing a corrupt
+ * stub (#566). Only invoked WITHOUT a trusted `generatedSource`; a materialized
+ * change is written verbatim and never reaches this guard. Two un-serialisable
+ * cases are detected (today scoped to `rule`, the only target scaffolded from a
+ * structured definition):
+ *
+ *   (a) The effective definition (`change.definition ?? {}`) lacks a required
+ *       field (trigger / condition / effect). This is the code-condition
+ *       `requiresCodeChange:true` case where the NL resolver intentionally left
+ *       `definition` undefined; the old `?? { name: change.name }` fallback would
+ *       have emitted `defineRule({ "name": "…" })` — a stub missing the rule's
+ *       behaviour. We refuse instead.
+ *   (b) A required field carries a FUNCTION (e.g. a `CodeCondition`).
+ *       `JSON.stringify` silently DROPS it, emitting a broken rule with the field
+ *       lost. We refuse rather than corrupt.
+ *
+ * The fix a caller is pointed at: a `requiresCodeChange` update needs a
+ * materialized `generatedSource` (the AI materializer), not deterministic codegen.
+ */
+function assertGraduatable(proposal: ProposalDefinition, change: ProposalChange): void {
+  const target = assertWritableTarget(change.target);
+  // Only `rule` is scaffolded from a structured definition with mandatory
+  // behavioural fields today; other targets ship a self-contained declarative
+  // definition or arrive with `generatedSource`. Keep the guard scoped so we
+  // never regress them.
+  if (target !== "rule") return;
+
+  const definition = (change.definition ?? {}) as Record<string, unknown>;
+  // Common prefix names the proposal id, change name and target (the required
+  // "what"); each branch appends the specific "why" + the materialization fix.
+  const where = `${target} "${change.name}" (proposal "${proposal.id}")`;
+  const fix =
+    "such a requiresCodeChange update needs a materialized `generatedSource` " +
+    "(the AI materializer), not deterministic codegen — refusing to write an invalid stub.";
+
+  for (const field of RULE_REQUIRED_FIELDS) {
+    const value = definition[field];
+    if (value === undefined || value === null) {
+      throw new Error(
+        `ProposalFileWriter: cannot deterministically serialize ${where} — its definition has ` +
+          `no "${field}" (the code-condition case where the NL resolver intentionally left the ` +
+          `definition undefined); ${fix}`,
+      );
+    }
+    if (typeof value === "function") {
+      throw new Error(
+        `ProposalFileWriter: cannot deterministically serialize ${where} — its "${field}" is a ` +
+          `function (code condition); JSON.stringify would silently drop it, emitting a rule ` +
+          `with "${field}" lost; ${fix}`,
+      );
+    }
+  }
+}
+
 /** Default codegen: import the matching `defineXxx` and re-emit the change. */
 function defaultCodegen(proposal: ProposalDefinition, change: ProposalChange): string {
   const factory = TARGET_FACTORY[assertWritableTarget(change.target)];
+  // `assertGraduatable` runs BEFORE codegen and refuses the un-serialisable
+  // cases, so a `rule` always carries trigger/condition/effect here. The
+  // `?? { name }` default only covers non-rule targets that serialise from a
+  // name-only definition.
   const definition = change.definition ?? { name: change.name };
   const header = buildHeader(proposal, change);
 
@@ -301,11 +364,21 @@ export class ProposalFileWriter {
   private readonly pathResolver?: (proposal: ProposalDefinition, change: ProposalChange) => string;
   private readonly codegen: (proposal: ProposalDefinition, change: ProposalChange) => string;
   private readonly formatter?: ProposalSourceFormatter;
+  /**
+   * Whether the deterministic {@link defaultCodegen} is in use (no custom
+   * `codegen` was supplied). The {@link assertGraduatable} guard encodes the
+   * constraints of `defaultCodegen` specifically (JSON.stringify drops
+   * functions; a rule needs trigger/condition/effect). A caller-supplied
+   * `codegen` is an escape hatch for unusual `ChangeDefinition` shapes and may
+   * handle exactly those cases — so the guard must NOT pre-empt it.
+   */
+  private readonly usesDefaultCodegen: boolean;
 
   constructor(options: ProposalFileWriterOptions) {
     this.rootDir = options.rootDir;
     this.logger = options.logger;
     this.pathResolver = options.pathResolver;
+    this.usesDefaultCodegen = options.codegen === undefined;
     this.codegen = options.codegen ?? defaultCodegen;
     this.formatter = resolveFormatter(options.formatter);
   }
@@ -373,9 +446,22 @@ export class ProposalFileWriter {
       // no generatedSource → codegen.
       const hasGeneratedSource =
         typeof change.generatedSource === "string" && change.generatedSource.trim().length > 0;
-      const rawSource = hasGeneratedSource
-        ? (change.generatedSource as string)
-        : this.codegen(proposal, change);
+      let rawSource: string;
+      if (hasGeneratedSource) {
+        rawSource = change.generatedSource as string;
+      } else {
+        // No trusted materialized source — deterministically serialize
+        // `change.definition`, but refuse FIRST if it can't round-trip to valid
+        // source so a code-condition / requiresCodeChange:true draft FAILS LOUD
+        // here instead of graduating a corrupt stub (#566). The guard only
+        // applies to `defaultCodegen`; a caller-supplied `codegen` is an escape
+        // hatch that may legitimately handle code-condition / function-bearing
+        // definitions itself, so we must not pre-empt it.
+        if (this.usesDefaultCodegen) {
+          assertGraduatable(proposal, change);
+        }
+        rawSource = this.codegen(proposal, change);
+      }
       const source = await this.maybeFormat(rawSource, targetPath, proposal);
       await mkdir(dirname(targetPath), { recursive: true });