Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sourcepatch-approval-validation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linchkit/core": patch
---

Fix Phase-1 proposal validation rejecting `sourcePatch`-carrying rule updates as `MISSING_DEFINITION` (#566). A "say→change an existing code-condition rule's threshold" proposal carries a definition-less change whose `sourcePatch` IS the specification (value-validated at assembly, re-validated by the patcher at graduation). Phase 1 now skips the definition requirement for such a change — like `revert`/`delete` — so the governed draft can reach the approval gate and graduate. Without this, every natural-language rule-threshold update failed Phase 1 and was unapprovable (the resolver produced a correct proposal that could never be approved). Caught by live testing of the chat-assistant approval flow.
46 changes: 46 additions & 0 deletions packages/core/__tests__/proposal-validation-phase1.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,52 @@ describe("validatePhase1", () => {
expect(result.status).toBe("passed");
expect(result.errors).toHaveLength(0);
});

it("passes a code-rule update carrying a sourcePatch despite having no definition (#566)", () => {
// A "say→change an existing code-condition rule's threshold" proposal carries
// a definition-less change whose `sourcePatch` IS the spec. Phase-1 must NOT
// flag MISSING_DEFINITION, or the NL rule-threshold update can never be
// approved and graduated (caught by live testing — the approve gate blocked it).
const result = validatePhase1({
changes: [
{
target: "rule",
operation: "update",
name: "manager_approval_threshold",
sourcePatch: {
filePath: "addons/demo/cap-purchase-demo/src/rules/manager-approval-threshold.ts",
constantName: "MANAGER_APPROVAL_THRESHOLD",
newValueLiteral: "20000",
},
},
],
});

expect(result.status).toBe("passed");
expect(result.errors).toHaveLength(0);
});

it("fails a code-rule update carrying an invalid (empty filePath) sourcePatch", () => {
// The carve-out skips MISSING_DEFINITION but still validates the patch's own
// shape: a malformed sourcePatch is caught at Phase 1, not opaquely at graduation.
const result = validatePhase1({
changes: [
{
target: "rule",
operation: "update",
name: "manager_approval_threshold",
sourcePatch: {
filePath: "",
constantName: "MANAGER_APPROVAL_THRESHOLD",
newValueLiteral: "20000",
},
},
],
});

expect(result.status).toBe("failed");
expect(result.errors.some((e) => e.code === "INVALID_SOURCE_PATCH")).toBe(true);
});
});
Comment thread
laofahai marked this conversation as resolved.

// ── validatePhase1: duplicate detection ─────────────────
Expand Down
21 changes: 21 additions & 0 deletions packages/core/src/engine/validation-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,27 @@ export function validatePhase1(options: {
// the target-specific definition validation below) just as we skip deletes,
// so a governance-safe draft rollback Proposal can reach the approval gate.
if (change.target === "revert") continue;
// A change carrying a `sourcePatch` (#566 "say→change an existing
// code-condition rule's threshold") intentionally has NO `definition`: it
// rewrites a single named constant in existing source, so the sourcePatch IS
// the specification (value-validated at assembly via `isSafeValueLiteral`,
// and the patcher re-validates at graduation). Skip the MISSING_DEFINITION
// requirement — like revert/delete — so the governed draft can reach the
// approval gate and graduate. Without this, every NL rule-threshold update
// fails Phase 1 and is unapprovable. Still validate the patch's OWN shape
// here so a malformed sourcePatch is caught early at Phase 1 rather than
// failing opaquely during graduation.
if (change.sourcePatch) {
Comment thread
laofahai marked this conversation as resolved.
Outdated
const { filePath, constantName, newValueLiteral } = change.sourcePatch;
if (!filePath || !constantName || !newValueLiteral) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

errors.push({
code: "INVALID_SOURCE_PATCH",
message: `Change for "${change.name}" has an invalid or incomplete sourcePatch specification`,
target: change.name,
});
}
continue;
}
if (!change.definition) {
errors.push({
code: "MISSING_DEFINITION",
Expand Down
Loading