Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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/vale-raw-authoring.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@taskless/cli": patch
---

`verify` warns when a Vale rule's `raw` list has more than one entry, since Vale concatenates them into one pattern rather than alternating them; the `create-vale-rule` recipe explains the `(a|b)` form. The warning rides on `notice` and does not fail the rule.
8 changes: 7 additions & 1 deletion packages/cli/src/commands/verify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,14 @@ async function runOverPath(options: {
// Printed even when the rule passed. A misplaced `.vale.ini` assignment
// makes Vale exit zero having enabled nothing, so the clean line above
// is exactly the moment the author needs to hear this.
//
// A notice is one advisory per line, joined with `\n` (a style-layer
// advisory beside a config-layer one, say). Every line gets the prefix,
// so the second reads as a notice rather than as an unlabelled stray.
if ("notice" in result && result.notice !== undefined) {
console.log(` notice: ${result.notice}`);
for (const line of result.notice.split("\n")) {
console.log(` notice: ${line}`);
}
}
}
// A rule that did not run is not among the rules tested. Counting it there
Expand Down
19 changes: 11 additions & 8 deletions packages/cli/src/rules/inspect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ export async function verifyOneRule(
}

if (engine === "vale") {
// What is true but not invalid, from the style file and the config alike,
// rides on `notice`. Both schema layers speak in that register: the style
// layer about a `raw` list Vale will join into one pattern, the config
// layer about a repeated key or a `[*]` matcher.
const advisories: string[] = [];
const stylePath = ruleFilePath(cwd, engine, ruleId);
try {
const style = await readYaml(stylePath);
Expand All @@ -216,7 +221,9 @@ export async function verifyOneRule(
// catches are not local — Vale reads one assembled config per run, so an
// unknown `extends` or a foreign field takes down every other Vale
// rule's findings rather than just this one's.
errors.push(...validateValeRule(ruleId, style).errors);
const schema = validateValeRule(ruleId, style);
errors.push(...schema.errors);
advisories.push(...schema.advisories);

if (typeof style === "object" && style !== null) {
const record = style as Record<string, unknown>;
Expand Down Expand Up @@ -246,10 +253,8 @@ export async function verifyOneRule(
// Vale would accept and read as something other than what its author
// wrote: a rule assignment above the first matcher, a matcher with no
// breadcrumb, a key naming another rule. Each rejection is attributed to
// its `vale-config-*` constraint; what is true but not invalid rides on
// `notice`.
// its `vale-config-*` constraint.
const violations: RuleViolation[] = [];
let notice: string | undefined;
const configPath = ruleConfigPath(cwd, engine, ruleId);
if (configPath !== undefined) {
let config: string | undefined;
Expand All @@ -269,9 +274,7 @@ export async function verifyOneRule(
rejection.message
);
}
if (verdict.advisories.length > 0) {
notice = verdict.advisories.join("\n");
}
advisories.push(...verdict.advisories);
}
}

Expand All @@ -284,7 +287,7 @@ export async function verifyOneRule(
ok: errors.length === 0,
errors,
violations,
...(notice === undefined ? {} : { notice }),
...(advisories.length === 0 ? {} : { notice: advisories.join("\n") }),
Comment thread
theCodeDrift marked this conversation as resolved.
};
}

Expand Down
64 changes: 58 additions & 6 deletions packages/cli/src/schemas/vale-rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -812,8 +812,57 @@ export const valeRuleSchema = valeHeaderSchema
.transform(canonicalKeys)
.pipe(valeBodySchema);

/** What the schema layer concluded. Shared with the ast-grep path. */
export type ValeSchemaResult = SchemaLayerResult;
/**
* What is true about a rule that does not make it invalid.
*
* The counterpart of `adviseValeRuleConfig` in `vale-config.ts`, and held to
* the same line: each entry has a legitimate reading, so none is a rejection.
* Said rather than refused, on `notice`.
*
* Read off the canonical keys, because `Raw:` is decoded case-insensitively
* like every non-literal field; and off the mapping whether or not the schema
* passed it, since an advisory about one field is as true beside a rejection
* of another as it is alone.
*/
function adviseValeRule(ruleId: string, data: unknown): string[] {
if (typeof data !== "object" || data === null || Array.isArray(data)) {
return [];
}
const rule = canonicalKeys(data as Record<string, unknown>);
Comment thread
theCodeDrift marked this conversation as resolved.
const advisories: string[] = [];

// Vale joins an `existence` rule's `raw` list with no separator
// (`strings.Join(rule.Raw, "")`), so two entries are one regex, not two
// alternatives, and a line matching only the second entry is silent. That
// is a real authoring trap (taskless/cli#360: the second entry's fail
// fixture never fired, with no error anywhere) and also a real way to write
// a long pattern across lines, which is why it is a notice. Pinned on the
// binary in `vale-vendor-contract.test.ts`, "existence `raw` entries join
// into one pattern".
if (
rule.extends === "existence" &&
Array.isArray(rule.raw) &&
rule.raw.length > 1
) {
advisories.push(
`${ruleId}: raw has ${String(rule.raw.length)} entries; Vale joins them into one pattern with no separator, so the second never matches on its own. Write one entry with (a|b) unless the join is intended.`
);
}
Comment thread
theCodeDrift marked this conversation as resolved.
return advisories;
}

/**
* What the schema layer concluded, plus what it noticed.
*
* `valid` and `errors` are the shape shared with the ast-grep path;
* `advisories` is the Vale layer's own, since its checks are hand-authored
* against a measured binary and some of what they know is worth saying without
* being worth failing.
*/
export interface ValeSchemaResult extends SchemaLayerResult {
/** True things about the rule that do not make it invalid. */
advisories: string[];
}

/**
* Validate a parsed Vale style file structurally, before Vale is invoked.
Expand All @@ -826,8 +875,11 @@ export function validateValeRule(
ruleId: string,
data: unknown
): ValeSchemaResult {
return schemaLayer(
valeRuleSchema.safeParse(data),
(issue) => `${ruleId}.yml: ${issue.message}`
);
return {
...schemaLayer(
valeRuleSchema.safeParse(data),
(issue) => `${ruleId}.yml: ${issue.message}`
),
advisories: adviseValeRule(ruleId, data),
};
}
82 changes: 82 additions & 0 deletions packages/cli/test/vale-schema-contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,88 @@ describe("the permissive checks stay permissive", () => {
});
});

/**
* What the schema says without failing the rule.
*
* The corpus has three verdicts and none of them is "accepted, with a note",
* so an advisory cannot be a row there; it is pinned here instead, beside the
* other tests that ask the schema rather than the binary. The behaviour the
* advisory describes is measured in `vale-vendor-contract.test.ts`
* ("existence `raw` entries join into one pattern"); this asks only that the
* schema says so, and only when it applies.
*/
describe("advisories are said, not refused", () => {
const RAW_TWO =
'extends: existence\nmessage: "x"\nlevel: warning\nraw:\n - "\\\\bstops being\\\\b"\n - "\\\\band becomes\\\\b"\n';

it("warns on a raw list with more than one entry, and still accepts the rule", () => {
const { valid, errors, advisories } = validateValeRule(
"no-twist",
parseYaml(RAW_TWO)
);
expect(valid).toBe(true);
expect(errors).toEqual([]);
expect(advisories).toEqual([
"no-twist: raw has 2 entries; Vale joins them into one pattern with no separator, so the second never matches on its own. Write one entry with (a|b) unless the join is intended.",
]);
});

it("counts the entries it saw", () => {
const { advisories } = validateValeRule(
"demo",
parseYaml('extends: existence\nmessage: "x"\nraw:\n - a\n - b\n - c\n')
);
expect(advisories[0]).toContain("raw has 3 entries");
});

it("is silent on a one-entry raw list", () => {
const { advisories } = validateValeRule(
"demo",
parseYaml(
'extends: existence\nmessage: "x"\nraw:\n - "(\\\\bstops being\\\\b|\\\\band becomes\\\\b)"\n'
)
);
expect(advisories).toEqual([]);
});

it("is silent on a tokens list, which Vale does alternate", () => {
const { advisories } = validateValeRule(
"demo",
parseYaml('extends: existence\nmessage: "x"\ntokens:\n - a\n - b\n')
);
expect(advisories).toEqual([]);
});

it("reads the key case-insensitively, as Vale decodes it", () => {
const { advisories } = validateValeRule(
"demo",
parseYaml('extends: existence\nmessage: "x"\nRaw:\n - a\n - b\n')
);
expect(advisories).toHaveLength(1);
});

it("still speaks beside a rejection of another field", () => {
// An advisory about `raw` is no less true because `scope` is wrong, and an
// author fixing the one should hear about the other in the same pass.
const { valid, advisories } = validateValeRule(
"demo",
parseYaml(
'extends: existence\nmessage: "x"\nscope: fenced\nraw:\n - a\n - b\n'
)
);
expect(valid).toBe(false);
expect(advisories).toHaveLength(1);
});

it("carries an empty list, never an absent one, when there is nothing to say", () => {
const { advisories } = validateValeRule(
"demo",
parseYaml('extends: existence\nmessage: "x"\ntokens: [a]\n')
);
expect(advisories).toEqual([]);
});
});

// --- Helpers -----------------------------------------------------------------

/**
Expand Down
Loading
Loading