Skip to content

Commit 0a66efe

Browse files
committed
fix(lint): slice 16 test pins follow the one-line fix lines (WIP)
Claude-Session: https://claude.ai/code/session_019G5ZnRrTMiwcHG6iZM19zf Co-authored-by: Claude <noreply@anthropic.com>
1 parent 54ff0ba commit 0a66efe

15 files changed

Lines changed: 735 additions & 102 deletions

‎packages/lint/src/rule-explanations.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3006,7 +3006,8 @@ const TRANSLATION_TARGET_UNKNOWN_EXPLANATION: RuleExplanation = {
30063006
'line names the remedy only, without listing what is declared; what each key may name follows.',
30073007
'Under `objects.OBJECT` (an object this stack, or a sibling package of the same artifact, ' +
30083008
'defines): `fields.FIELD` a declared field (a system column, `_id`, `name` and `space` are ' +
3009-
'never reported); `_views.VIEW` a view\'s `name`, not its label; `_sections.KEY` a ' +
3009+
'never reported); `_views.VIEW` a view\'s `name`, not its label — the runtime keys a nameless ' +
3010+
'default `list` `default`, never `list`; `_sections.KEY` a ' +
30103011
'`fieldGroups[].key`, a named form-view section or a named `record:details` section — a ' +
30113012
'section is translatable only through a stable name; `_tabs.TAB` a filter-preset tab that a ' +
30123013
'page over the object declares in `interfaceConfig.userFilters.tabs` (a list view\'s own ' +

‎packages/lint/src/validate-ai-surface-affinity.test.ts‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { explainRule } from './rule-explanations.js';
1313
// verdict to one line of at most 200 characters — every firing variant this
1414
// suite exercises, not a chosen few. Run the whole file: those cases read what
1515
// the cases above fired.
16-
const fired: Array<{ rule: string; message: string }> = [];
16+
const fired: Array<{ rule: string; message: string; hint: string }> = [];
1717
const validateAiSurfaceAffinity: typeof validateAiSurfaceAffinityUnrecorded = (...args) => {
1818
const findings = validateAiSurfaceAffinityUnrecorded(...args);
1919
fired.push(...findings);
@@ -196,3 +196,30 @@ describe('[#22161] one-line verdicts — ai-skill-surface-mismatch', () => {
196196
}
197197
});
198198
});
199+
200+
// [#22161] Slice 16: the `fix:` line names the three ways out in one line; when
201+
// `surface: 'both'` is right is the explanation's.
202+
describe('[#22161] one-line fix — ai-skill-surface-mismatch', () => {
203+
it('reads as one remedy, a long skill name included, at most 200 characters', () => {
204+
const ref = 'regional_enterprise_pipeline_forecast_reviewer';
205+
const [f] = validateAiSurfaceAffinity({
206+
agents: [{ name: 'sales_assistant', surface: 'ask', skills: [ref] }],
207+
skills: [{ name: ref, surface: 'build' }],
208+
});
209+
expect(f!.hint).toBe(
210+
`Move "${ref}" to a 'build'-surface agent, change its \`surface\` to 'ask', or declare \`surface: 'both'\` ` +
211+
'for a shared, read-only skill',
212+
);
213+
const converted = fired.filter((x) => x.rule === AI_SKILL_SURFACE_MISMATCH);
214+
expect(converted.length).toBeGreaterThan(1);
215+
for (const x of converted) {
216+
expect(x.hint, x.rule).not.toContain('\n');
217+
expect(x.hint.length, `${x.rule}: ${x.hint}`).toBeLessThanOrEqual(200);
218+
}
219+
});
220+
221+
it('`os explain` carries what the `fix:` line no longer says', () => {
222+
const text = explainRule(AI_SKILL_SURFACE_MISMATCH)!.paragraphs.join('\n');
223+
expect(text).toContain("Declare `surface: 'both'` only for a genuinely shared, read-only capability");
224+
});
225+
});

‎packages/lint/src/validate-approval-approvers.test.ts‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import { describe, it, expect } from 'vitest';
44
import { BUILTIN_MEMBERSHIP_ROLES } from '@objectstack/spec';
5+
import { APPROVER_EXPRESSION_ROOTS, ApprovalNodeConfigSchema } from '@objectstack/spec/automation';
56
import {
67
validateApprovalApprovers as validateApprovalApproversUnrecorded,
78
APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER,
@@ -856,3 +857,127 @@ describe('[#22161] one-line fixes — approval-approvers-may-resolve-empty, agai
856857
expect(text).toContain("only an organization with no administrator leaves the request on an empty");
857858
});
858859
});
860+
861+
// [#22161] Slice 16: the `fix:` lines of the other three `approval-*` ids. Each
862+
// names its remedy in one line; what each root holds, why `record` is unbound,
863+
// what each empty-slate policy does and why a business role is a position are
864+
// the explanations'. The bound reads every finding the cases above fired.
865+
describe('[#22161] one-line fixes — the expression and membership-tier ids', () => {
866+
const SLICE_16 = [
867+
APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER,
868+
APPROVAL_EXPRESSION_INVALID,
869+
APPROVAL_EXPRESSION_NO_EMPTY_POLICY,
870+
];
871+
const stackWithConfig = (config: Record<string, unknown>): Record<string, unknown> => ({
872+
flows: [{
873+
name: 'expense_approval',
874+
nodes: [
875+
{ id: 'start', type: 'start', config: {} },
876+
{ id: 'step1', type: 'approval', config },
877+
],
878+
edges: [],
879+
}],
880+
});
881+
882+
it('every `fix:` line of the three ids the cases above fired is one line of at most 200 characters', () => {
883+
// A long position name, so the bound is not read on short names alone.
884+
validateApprovalApprovers(stackWithApprovers([
885+
{ type: 'org_membership_level', value: 'regional_vice_president_of_enterprise_sales' },
886+
]));
887+
const converted = fired.filter((f) => SLICE_16.includes(f.rule));
888+
// Coverage control: every arm of the three ids fired.
889+
expect(converted.some((f) => f.rule === APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER)).toBe(true);
890+
expect(converted.some((f) => f.rule === APPROVAL_EXPRESSION_NO_EMPTY_POLICY)).toBe(true);
891+
const invalid = converted.filter((f) => f.rule === APPROVAL_EXPRESSION_INVALID);
892+
for (const arm of ['empty expression', 'does not parse', 'references `record`', 'references `approvers_dynamic`', 'resolveAs has no effect']) {
893+
expect(invalid.some((f) => f.message.includes(arm)), arm).toBe(true);
894+
}
895+
for (const f of converted) {
896+
expect(f.hint, f.rule).not.toContain('\n');
897+
expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200);
898+
}
899+
});
900+
901+
it('each arm reads as one remedy', () => {
902+
const [tier] = validateApprovalApprovers(stackWithApprovers([{ type: 'org_membership_level', value: 'sales_manager' }]));
903+
expect(tier.hint).toBe(
904+
"If it is an org position, author { type: 'position', value: 'sales_manager' }; keep " +
905+
"'org_membership_level' for the tiers the verdict lists",
906+
);
907+
const [empty] = validateApprovalApprovers(stackWithConfig({
908+
approvers: [{ type: 'expression', value: '' }],
909+
onEmptyApprovers: 'fail',
910+
}));
911+
expect(empty.hint).toBe('Write a CEL expression over current.*, trigger.* or vars.*, e.g. current.approvers_dynamic');
912+
const [record] = validateApprovalApprovers(stackWithConfig({
913+
approvers: [{ type: 'expression', value: 'record.owner_id' }],
914+
onEmptyApprovers: 'fail',
915+
}));
916+
expect(record.hint).toBe(
917+
'Write current.<field> for the live value at node entry or trigger.<field> for the submit-time ' +
918+
'snapshot; vars.previous carries the pre-update row',
919+
);
920+
const [nudge] = validateApprovalApprovers(stackWithConfig({
921+
approvers: [{ type: 'expression', value: 'vars.picked' }],
922+
}));
923+
expect(nudge.hint).toBe(
924+
"Declare onEmptyApprovers on the node: 'admin_rescue', 'fail', 'auto_approve', or 'fallback' " +
925+
'with a fallbackApprovers list',
926+
);
927+
});
928+
929+
it("the nudge's policies and the explanation's are the spec's `onEmptyApprovers` enum", () => {
930+
// The explanation module imports nothing, so it spells the list out; this
931+
// holds both it and the fix line to the schema.
932+
const policies = (
933+
ApprovalNodeConfigSchema.shape.onEmptyApprovers.unwrap() as unknown as { options: readonly string[] }
934+
).options;
935+
expect([...policies].sort()).toEqual(['admin_rescue', 'auto_approve', 'fail', 'fallback']);
936+
const [nudge] = validateApprovalApprovers(stackWithConfig({
937+
approvers: [{ type: 'expression', value: 'vars.picked' }],
938+
}));
939+
const text = explainRule(APPROVAL_EXPRESSION_NO_EMPTY_POLICY)!.paragraphs.join('\n');
940+
for (const policy of policies) {
941+
expect(nudge.hint, policy).toContain(`'${policy}'`);
942+
expect(text, policy).toContain(`\`${policy}\``);
943+
}
944+
});
945+
946+
it("the explanation's expression roots are the spec's `APPROVER_EXPRESSION_ROOTS`", () => {
947+
const text = explainRule(APPROVAL_EXPRESSION_INVALID)!.paragraphs.join('\n');
948+
expect(APPROVER_EXPRESSION_ROOTS.length).toBe(3);
949+
for (const root of APPROVER_EXPRESSION_ROOTS) expect(text, root).toContain(`\`${root}.*\``);
950+
});
951+
952+
it('`os explain` carries what the three `fix:` lines no longer say', () => {
953+
const facts: Record<string, string[]> = {
954+
[APPROVAL_EXPRESSION_INVALID]: [
955+
"record's live state at node entry",
956+
'the submit-time snapshot',
957+
'no `{…}` template braces',
958+
'record at event time',
959+
'`vars.previous` carries the pre-update row',
960+
'fails the node at entry',
961+
'`resolveAs` means something only on an `expression` approver',
962+
],
963+
[APPROVAL_EXPRESSION_NO_EMPTY_POLICY]: [
964+
"the organization's administrators",
965+
'its fault edge',
966+
'`output.autoApproved = true`',
967+
'`fallbackApprovers`',
968+
'degrades to `admin_rescue`',
969+
],
970+
[APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER]: [
971+
'A business role is always a position',
972+
'`sys_user_position`',
973+
'ADR-0108',
974+
],
975+
};
976+
for (const [rule, list] of Object.entries(facts)) {
977+
const entry = explainRule(rule);
978+
expect(entry, `no \`os explain ${rule}\` entry`).toBeDefined();
979+
const text = entry!.paragraphs.join('\n');
980+
for (const fact of list) expect(text, `${rule} explanation names ${fact}`).toContain(fact);
981+
}
982+
});
983+
});

‎packages/lint/src/validate-approval-approvers.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin
460460
// and why a business role is always a position (the closed
461461
// ADR-0108 vocabulary, `sys_user_position`) is the explanation's.
462462
hint:
463-
`If '${value}' is an org position, author { type: 'position', value: '${value}' }; keep ` +
463+
`If it is an org position, author { type: 'position', value: '${value}' }; keep ` +
464464
`'org_membership_level' for the tiers the verdict lists`,
465465
});
466466
} else if (type in DEPRECATED_APPROVER_TYPES) {

‎packages/lint/src/validate-chart-bindings.test.ts‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { explainRule } from './rule-explanations.js';
1616
// verdict of those ids to one line of at most 200 characters — every position
1717
// and surface this suite exercises, not a chosen few. Run the whole file: those
1818
// cases read what the cases above fired.
19-
const fired: Array<{ rule: string; message: string; path: string }> = [];
19+
const fired: Array<{ rule: string; message: string; path: string; hint: string }> = [];
2020
const validateChartBindings: typeof validateChartBindingsUnrecorded = (...args) => {
2121
const findings = validateChartBindingsUnrecorded(...args);
2222
fired.push(...findings);
@@ -58,7 +58,9 @@ describe('validateChartBindings — report charts', () => {
5858
expect(findings[0].rule).toBe(CHART_MEASURE_UNKNOWN);
5959
expect(findings[0].path).toBe('reports[0].chart.yAxis');
6060
expect(findings[0].message).toContain('estimate_hours');
61-
expect(findings[0].hint).toContain('est_hours');
61+
// [#22161] The remedy alone: the dataset's measure list is no longer
62+
// appended, and `estimate_hours` is no near miss of a declared measure.
63+
expect(findings[0].hint).toBe('Bind a measure that dataset "task_metrics" declares, or declare "estimate_hours" on it.');
6264
});
6365

6466
// #14577 — this rule used to carry a private Levenshtein-only `suggest`,
@@ -547,8 +549,11 @@ describe('validateChartBindings — binding vs presentation positions (#15575)',
547549
expect(findings[0].message).toContain('lands on nothing');
548550
expect(findings[0].message).not.toContain('comes back empty');
549551
// No surface here carries `suppressWarnings` (dashboard widgets only), and
550-
// the hint says so rather than advertising a key that does not exist.
551-
expect(findings[0].hint).toContain('no `suppressWarnings` key');
552+
// nothing says otherwise: [#22161] the hint names the remedy alone, and the
553+
// explanation says no key silences the advisory.
554+
expect(findings[0].hint).toBe('Correct the name to a measure that dataset "task_metrics" declares, or drop the entry.');
555+
expect(findings[0].hint).not.toContain('suppressWarnings');
556+
expect(explainRule(CHART_MEASURE_UNKNOWN)!.paragraphs.join('\n')).toContain('carries a `suppressWarnings` key');
552557
});
553558

554559
it('one report chart reports BOTH tiers — the query position gates, the presentation position advises', () => {
@@ -1160,3 +1165,48 @@ describe('[#22161] one-line verdicts — chart-measure-unknown, chart-axis-not-s
11601165
expect(last(CHART_AXIS_NOT_SELECTED)).toBe(last(CHART_MEASURE_UNKNOWN));
11611166
});
11621167
});
1168+
1169+
// [#22161] Slice 16: the `fix:` line of chart-measure-unknown. One remedy line
1170+
// per position kind plus the nearest declared measure; the dataset's measure
1171+
// list is no longer appended, and what each presentation position carries is
1172+
// the explanation's.
1173+
describe('[#22161] one-line fix — chart-measure-unknown', () => {
1174+
it('every `fix:` line the cases above fired is one line of at most 200 characters', () => {
1175+
const converted = fired.filter((f) => f.rule === CHART_MEASURE_UNKNOWN);
1176+
// Coverage control: a query position and every presentation position, a
1177+
// near miss and none.
1178+
for (const marker of ['comes back empty', 'DISPLAY-NAME override', 'REPLACES the authored array', 'axis PRESENTATION']) {
1179+
expect(converted.some((f) => f.message.includes(marker)), marker).toBe(true);
1180+
}
1181+
expect(converted.some((f) => f.hint.includes('Did you mean'))).toBe(true);
1182+
expect(converted.some((f) => !f.hint.includes('Did you mean'))).toBe(true);
1183+
for (const f of converted) {
1184+
expect(f.hint, f.rule).not.toContain('\n');
1185+
expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200);
1186+
expect(f.hint, f.rule).not.toContain('Dataset measures:');
1187+
}
1188+
});
1189+
1190+
it('keeps the nearest declared measure after the remedy', () => {
1191+
const [f] = validateChartBindings({
1192+
datasets: [{ name: 'sales_metrics', object: 'o', dimensions: [{ name: 'stage', field: 'stage' }], measures: [{ name: 'sum_amount', aggregate: 'sum', field: 'amount' }] }],
1193+
reports: [{ name: 'r', dataset: 'sales_metrics', values: ['sum_amount'], chart: { type: 'bar', xAxis: 'stage', yAxis: 'amount' } }],
1194+
});
1195+
expect(f!.hint).toBe(
1196+
'Bind a measure that dataset "sales_metrics" declares, or declare "amount" on it. Did you mean "sum_amount"?',
1197+
);
1198+
});
1199+
1200+
it('`os explain` carries what the presentation `fix:` line no longer says', () => {
1201+
const text = explainRule(CHART_MEASURE_UNKNOWN)!.paragraphs.join('\n');
1202+
for (const fact of [
1203+
'cannot add, remove or re-point one',
1204+
'membership belongs to the dataset selection',
1205+
'title, min/max, position, gridlines',
1206+
'carries a `suppressWarnings` key',
1207+
'"Did you mean"',
1208+
]) {
1209+
expect(text, fact).toContain(fact);
1210+
}
1211+
});
1212+
});

‎packages/lint/src/validate-dataset-references.test.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ const SHORTENED_RULE_IDS: readonly string[] = [
2323
DATASET_FIELD_NOT_INCLUDED,
2424
DATASET_FILTER_FIELD_UNKNOWN,
2525
];
26-
const firedShortened: Array<{ rule: string; message: string }> = [];
26+
const firedShortened: Array<{ rule: string; message: string; hint: string }> = [];
2727
const validateDatasetReferences: typeof validateDatasetReferencesUnrecorded = (...args) => {
2828
const findings = validateDatasetReferencesUnrecorded(...args);
2929
for (const f of findings) if (SHORTENED_RULE_IDS.includes(f.rule)) firedShortened.push(f);
@@ -609,3 +609,63 @@ describe('[#22161] one-line verdicts — the rule ids this file shortened', () =
609609
for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact);
610610
});
611611
});
612+
613+
// [#22161] Slice 16: the `fix:` lines of the include and field ids. Each is the
614+
// remedy alone: the verdict's "Did you mean" names the nearest field, so the
615+
// object's field list and the traversable types are no longer appended; the
616+
// three-hop limit and what joins are the explanation's.
617+
describe('[#22161] one-line fixes — dataset-include-unknown and dataset-field-unknown', () => {
618+
const SLICE_16 = [DATASET_INCLUDE_UNKNOWN, DATASET_FIELD_UNKNOWN];
619+
620+
it('every `fix:` line of the two ids the cases above fired is one line of at most 200 characters', () => {
621+
const converted = firedShortened.filter((f) => SLICE_16.includes(f.rule));
622+
// Coverage control: every arm fired — an include entry that resolves to an
623+
// ordinary field, one that names nothing, one whose hop is not a
624+
// relationship; a dimension, a measure and a joined field path.
625+
const include = converted.filter((f) => f.rule === DATASET_INCLUDE_UNKNOWN);
626+
expect(include.some((f) => f.message.includes('not a relationship, so no join'))).toBe(true);
627+
expect(include.some((f) => f.message.includes('Did you mean "duty"?'))).toBe(true);
628+
expect(include.some((f) => f.message.includes('nothing to join through'))).toBe(true);
629+
const field = converted.filter((f) => f.rule === DATASET_FIELD_UNKNOWN);
630+
expect(field.some((f) => f.message.startsWith('dimension field'))).toBe(true);
631+
expect(field.some((f) => f.message.startsWith('measure field'))).toBe(true);
632+
expect(field.some((f) => f.message.includes('object "duly_duty"'))).toBe(true);
633+
for (const f of converted) {
634+
expect(f.hint, f.rule).not.toContain('\n');
635+
expect(f.hint.length, `${f.rule}: ${f.hint}`).toBeLessThanOrEqual(200);
636+
// The object's field list is the verdict's "Did you mean", not the fix line's.
637+
expect(f.hint, f.rule).not.toContain('Fields on');
638+
}
639+
});
640+
641+
it('each reads as one remedy', () => {
642+
const [include] = validateDatasetReferences(stackWith({ include: ['dutee'], dimensions: [], measures: [] }));
643+
expect(include.hint).toBe(
644+
'Name a relationship field on the join chain from "duly_task" (up to 3 hops), or drop the entry',
645+
);
646+
const [dimension] = validateDatasetReferences(
647+
stackWith({ dimensions: [{ name: 'period', field: 'period_kee' }], measures: [] }),
648+
);
649+
expect(dimension.hint).toBe(
650+
'Bind the dimension to a field on "duly_task", or to a `relationship[.relationship].field` path ' +
651+
'whose prefix is declared in `include`.',
652+
);
653+
});
654+
655+
it('`os explain` carries what the two `fix:` lines no longer say', () => {
656+
const facts: Record<string, readonly string[]> = {
657+
[DATASET_INCLUDE_UNKNOWN]: [
658+
'up to three hops (ADR-0071)',
659+
'a lookup, master_detail, user or tree field',
660+
'Declaring `a.b` implicitly includes `a`',
661+
'never an ON clause',
662+
'"Did you mean"',
663+
],
664+
[DATASET_FIELD_UNKNOWN]: ['the hop that failed', '"Did you mean"'],
665+
};
666+
for (const [rule, list] of Object.entries(facts)) {
667+
const text = explainRule(rule)!.paragraphs.join('\n');
668+
for (const fact of list) expect(text, `${rule} explanation names ${fact}`).toContain(fact);
669+
}
670+
});
671+
});

0 commit comments

Comments
 (0)