Skip to content

Commit f89dd33

Browse files
claude[bot]claude
andauthored
fix(cli, lint): gate a dangling lookup/master_detail reference, and resolve one across the artifact packages[] (#17066)
* feat(lint): enrol a field's reference target in object-reference-unknown's severity ladder Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x * feat(cli, lint): resolve a field reference against the artifact's packages[] on os build's per-package leg ADR-0130 makes the release artifact the co-ownership boundary, so `os build`'s per-package authoring-rule leg judging a package with no sight of its siblings is the run's defect, not the author's. `compile.ts` now hands each package's stack the artifact's `packages[]` as resolution context, and `validate-object-references` widens rung 1 to the names those entries provide. It changes what a rule can RESOLVE, never what it JUDGES: the collections read off the top level are still the one package's, and a name no entry provides still lands on rung 2 and errors. Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com> * docs(changeset): state the artifact-packages rung and the cli half Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1c83ca2 commit f89dd33

4 files changed

Lines changed: 410 additions & 7 deletions

File tree

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
---
2+
'@objectstack/lint': minor
3+
'@objectstack/cli': patch
4+
---
5+
6+
`object-reference-unknown` now judges a field's `reference` — the target of `Field.lookup()` / `Field.masterDetail()` / `Field.user()` — with the same four-rung ladder it applies to every other object-name site, and `os build`'s per-package run resolves those names across the artifact's `packages[]`
7+
8+
`FieldSchema.reference` is `z.string()`: the schema holds it present and non-empty on `lookup` / `master_detail`, and nothing anywhere asked whether the name resolved. So `os validate`, `os lint` and `os build` all exited 0 — no diagnostic of any severity — on `Field.lookup('zzz_object_that_does_not_exist')` (measured on 17.3.0), and the miss surfaced only at runtime: the record picker asking the REST layer for an object that is not registered (404 `OBJECT_NOT_FOUND`), `$expand` failing on the field, the form rendering a control that can never resolve a value.
9+
10+
The site joins `validateObjectReferences` and rides its existing ladder, so the three commands judge it identically:
11+
12+
1. resolves in the stack's own objects, or in the objects an entry of this artifact's `packages[]` provides → ok;
13+
2. resolves in `PLATFORM_PROVIDED_OBJECT_NAMES` (`sys_user`, the target `Field.user()` writes) → ok;
14+
3. unresolved and not platform-prefixed → **`error`** — `os validate` / `os build` / `os lint` exit 1;
15+
4. unresolved, platform-prefixed, registered by nothing (`sys_approval_process`) → the existing `object-reference-unregistered-platform` advisory.
16+
17+
Judged: `lookup`, `master_detail`, `user`. Not judged, on purpose: `tree` (the object schema already refuses any target but the own name), a `reference` on a non-relationship type (inert), and `objectExtensions[].fields` (an extension targets an object another package owns, routinely one this artifact does not carry).
18+
19+
## Migration
20+
21+
**A build that used to pass can now fail.** Rung 3 is a new `error`-level refusal on a published accept set. Point the field at one of the stack's own objects, at an object another package of the same artifact ships, or at a platform object by its full name (`sys_user`, not `user`); the finding names the objects that resolve and suggests the nearest one.
22+
23+
**A reference into a sibling package of the same release artifact resolves — it needs no annotation.** ADR-0130 makes the release artifact the co-ownership boundary, so `os build`'s per-package leg now hands each package's stack the artifact's `packages[]` as resolution context (`compile.ts`). A module's `crm_order.account` → its App package's `crm_account` is an ordinary rung-1 resolution on all three commands. This changes what a rule can resolve, never what it judges: the collections judged per package are still that package's own, and a name no entry of `packages[]` provides still errors on the per-package run exactly as it does on the union one.
24+
25+
**A reference into another RELEASE ARTIFACT still has no rung** — an app naming an object a separate product ships (HotCLM's `clm_contract.crm_contract` → HotCRM). It is unresolved and unprefixed, so rung 3 refuses it. The declared escape for that case resolves against declared manifest dependencies and is its own change; ⛔ it is deliberately not an authored per-field marker, which would be a one-line switch that silences the gate.

‎packages/cli/src/commands/compile.ts‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,40 @@ function artifactPackages(parsed: Record<string, unknown>): Array<{
9595
* would REFUSE, by name, every collection key this superset deliberately puts
9696
* under `manifest` — and re-opening it to stop the refusal would re-open the
9797
* real manifest surface with it.
98+
*
99+
* ## `packages` — the second key, and the only one (#16611)
100+
*
101+
* The body carries the collections this package OWNS. Judged with nothing else,
102+
* a rule that resolves an object NAME concludes that a name a SIBLING package in
103+
* the same artifact ships exists nowhere: `examples/app-multi-package`'s
104+
* `crm_order.account` → `crm_account` — a lookup that fixture's README
105+
* documents as the point of the fixture — is owned by its `core` package and
106+
* read from its `orders` one. ADR-0130 makes the release artifact the
107+
* co-ownership boundary, so a per-package pass that cannot see a sibling
108+
* package's objects is THIS RUN's defect and not the author's; the ruled fix
109+
* (director seat, decision batch #86) hands the per-package stack the
110+
* artifact's own `packages[]` as RESOLUTION CONTEXT.
111+
*
112+
* Two properties make that safe, and they are the whole design:
113+
*
114+
* 1. ⛔ It changes what a rule can RESOLVE, never what it JUDGES. The
115+
* collections read off the top level are still this package's alone, so
116+
* every per-package finding this leg exists to produce is still produced.
117+
* ⛔ This is NOT "skip the site per package" — that would silence the gate
118+
* on the one command that ships.
119+
* 2. A name no entry of `packages[]` provides resolves in neither run, so a
120+
* genuinely dangling reference still errors here exactly as it does in the
121+
* union run above.
122+
*
123+
* The array is passed through verbatim rather than reduced to a name list: the
124+
* consuming rule owns which of an entry's contents are resolution context, and a
125+
* set computed here would be a second copy of that decision, free to drift.
98126
*/
99-
function packageBodyAsStack(body: Record<string, unknown>): Record<string, unknown> {
100-
return { ...body, manifest: body };
127+
function packageBodyAsStack(
128+
body: Record<string, unknown>,
129+
artifactPackageEntries: unknown,
130+
): Record<string, unknown> {
131+
return { ...body, manifest: body, packages: artifactPackageEntries };
101132
}
102133

103134
/** Identity of one finding, for the per-package de-duplication below. */
@@ -437,6 +468,13 @@ export default class Compile extends Command {
437468
// every finding twice and the author cannot tell a real per-package
438469
// finding from an echo. What survives the filter is exactly the set
439470
// the union could not see.
471+
//
472+
// [#16611] Each package's stack is handed the artifact's `packages[]`
473+
// as RESOLUTION CONTEXT — see `packageBodyAsStack`. The list read here
474+
// is the one `artifactPackages` above walked, off the same parsed
475+
// stack, so the context a package resolves against is exactly the set
476+
// of packages this artifact will register (ADR-0130 D4/D5).
477+
const artifactPackageEntries = (result.data as Record<string, unknown>).packages;
440478
const packageEntries = artifactPackages(result.data as Record<string, unknown>);
441479
if (packageEntries.length > 0) {
442480
if (!flags.json) {
@@ -445,7 +483,7 @@ export default class Compile extends Command {
445483
const alreadyReported = new Set(findings.map(findingKey));
446484
const perPackageErrors: Array<{ package: string } & typeof ruleErrors[number]> = [];
447485
for (const pkg of packageEntries) {
448-
const asStack = packageBodyAsStack(pkg.body);
486+
const asStack = packageBodyAsStack(pkg.body, artifactPackageEntries);
449487
const pkgFindings = runAuthoringRules('build', {
450488
normalized: asStack,
451489
parsed: asStack,

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

Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,231 @@ describe('validateObjectReferences — action params', () => {
8080
});
8181
});
8282

83+
describe('validateObjectReferences — field relationship targets (#16611)', () => {
84+
// The card's control probe, verbatim: on 17.3.0 `os validate`, `os lint` and
85+
// `os build` all exited 0 on it, with no diagnostic of any severity.
86+
it('errors on a lookup whose reference names an object declared nowhere', () => {
87+
const stack = baseStack();
88+
(stack.objects[0].fields as Record<string, unknown>).zzz_probe = {
89+
type: 'lookup',
90+
label: 'Control probe',
91+
reference: 'zzz_object_that_does_not_exist',
92+
};
93+
const findings = validateObjectReferences(stack);
94+
expect(findings).toHaveLength(1);
95+
expect(findings[0].severity).toBe('error');
96+
expect(findings[0].rule).toBe(OBJECT_REFERENCE_UNKNOWN);
97+
expect(findings[0].where).toBe('object "crm_lead" · field "zzz_probe"');
98+
expect(findings[0].path).toBe('objects[0].fields.zzz_probe.reference');
99+
expect(findings[0].message).toContain('lookup target "zzz_object_that_does_not_exist"');
100+
// The hint says what the miss costs at runtime, not just that it is a miss.
101+
expect(findings[0].hint).toContain('record picker');
102+
});
103+
104+
it('errors on a master_detail whose reference names an object declared nowhere', () => {
105+
const stack = baseStack();
106+
(stack.objects[0].fields as Record<string, unknown>).parent = {
107+
type: 'master_detail',
108+
required: true,
109+
reference: 'crm_led',
110+
};
111+
const findings = validateObjectReferences(stack);
112+
expect(findings).toHaveLength(1);
113+
expect(findings[0].severity).toBe('error');
114+
expect(findings[0].path).toBe('objects[0].fields.parent.reference');
115+
expect(findings[0].message).toContain('master_detail target "crm_led"');
116+
// One edit away from an own object: the suggester names it.
117+
expect(findings[0].message).toContain('Did you mean "crm_lead"?');
118+
});
119+
120+
it('accepts a lookup into an own object and the `Field.user()` shape (rungs ① and ③)', () => {
121+
const stack = baseStack();
122+
Object.assign(stack.objects[0].fields as Record<string, unknown>, {
123+
account: { type: 'lookup', reference: 'crm_account' },
124+
owner: { type: 'user', reference: 'sys_user' },
125+
watchers: { type: 'lookup', reference: 'sys_user', multiple: true },
126+
});
127+
expect(validateObjectReferences(stack)).toEqual([]);
128+
});
129+
130+
it('warns (not errors) on a platform-shaped target no package registers (rung ④)', () => {
131+
const stack = baseStack();
132+
(stack.objects[0].fields as Record<string, unknown>).approval = {
133+
type: 'lookup',
134+
reference: 'sys_approval_process',
135+
};
136+
const findings = validateObjectReferences(stack);
137+
expect(findings).toHaveLength(1);
138+
expect(findings[0].severity).toBe('warning');
139+
expect(findings[0].rule).toBe(OBJECT_REFERENCE_UNREGISTERED_PLATFORM);
140+
expect(findings[0].path).toBe('objects[0].fields.approval.reference');
141+
expect(findings[0].hint).toContain('sys_approval_request');
142+
});
143+
144+
it('leaves the `reference` key alone on `tree` and on non-relationship types', () => {
145+
// `tree`: the schema already refuses any target but the own name
146+
// (`refuseForeignTreeReference`), so a survivor always resolves.
147+
// `text`: the key is inert there; a finding would be about the wrong thing.
148+
const stack = baseStack();
149+
Object.assign(stack.objects[0].fields as Record<string, unknown>, {
150+
parent: { type: 'tree', reference: 'crm_lead' },
151+
note: { type: 'text', reference: 'zzz_object_that_does_not_exist' },
152+
});
153+
expect(validateObjectReferences(stack)).toEqual([]);
154+
});
155+
156+
it('does not walk objectExtensions[].fields — cross-package by construction', () => {
157+
// An extension adds fields to an object ANOTHER package owns; its targets
158+
// are the cross-package case this ladder has no rung for, and the declared
159+
// escape is its own card. Pinned so the boundary is a decision, not a gap.
160+
const findings = validateObjectReferences({
161+
...baseStack(),
162+
objectExtensions: [
163+
{
164+
object: 'crm_contract',
165+
fields: { clause: { type: 'lookup', reference: 'zzz_object_that_does_not_exist' } },
166+
},
167+
],
168+
});
169+
expect(findings).toEqual([]);
170+
});
171+
172+
it('walks array-shaped and map-shaped field collections alike', () => {
173+
const findings = validateObjectReferences({
174+
objects: [
175+
{ name: 'crm_lead', fields: [{ name: 'owner', type: 'lookup', reference: 'user' }] },
176+
{ name: 'crm_account', fields: { rep: { type: 'lookup', reference: 'user' } } },
177+
],
178+
});
179+
expect(findings.map((f) => f.path)).toEqual([
180+
'objects[0].fields.owner.reference',
181+
'objects[1].fields.rep.reference',
182+
]);
183+
for (const f of findings) {
184+
expect(f.severity).toBe('error');
185+
expect(f.hint).toContain('sys_user');
186+
}
187+
});
188+
189+
it('reports the field site before the sites that hang off the same object', () => {
190+
const stack = baseStack();
191+
(stack.objects[0].fields as Record<string, unknown>).zzz_probe = {
192+
type: 'lookup',
193+
reference: 'zzz_object_that_does_not_exist',
194+
};
195+
(stack.objects[0] as Record<string, unknown>).actions = [
196+
{ name: 'convert', params: [{ name: 'target', type: 'lookup', reference: 'accounts' }] },
197+
];
198+
expect(validateObjectReferences(stack).map((f) => f.path)).toEqual([
199+
'objects[0].fields.zzz_probe.reference',
200+
'objects[0].actions[0].params[0].reference',
201+
]);
202+
});
203+
});
204+
205+
describe('validateObjectReferences — artifact packages[] as resolution context (#16611)', () => {
206+
/**
207+
* The shape `compile.ts`'s per-package leg hands the rules (ADR-0130 D4): ONE
208+
* package's collections at the top level, and the whole artifact's
209+
* `packages[]` beside them as context. Modelled on
210+
* `examples/app-multi-package`, whose `orders` package reads `crm_account`
211+
* out of its `core` sibling.
212+
*/
213+
const ORDERS_BODY = {
214+
id: 'com.example.multi.orders',
215+
objects: [
216+
{
217+
name: 'crm_order',
218+
fields: {
219+
number: { type: 'text' },
220+
account: { type: 'lookup', reference: 'crm_account' },
221+
},
222+
},
223+
],
224+
};
225+
const CORE_BODY = {
226+
id: 'com.example.multi.core',
227+
objects: [{ name: 'crm_account', fields: { name: { type: 'text' } } }],
228+
};
229+
const perPackageStack = (body: Record<string, unknown>, packages: unknown) => ({
230+
...body,
231+
manifest: body,
232+
packages,
233+
});
234+
235+
it('CONTROL — the same package judged ALONE still errors, so the context is what does the work', () => {
236+
// Without this leg "green" is indistinguishable from the rung having been
237+
// switched off: this is the exact finding that reds `Build Core` on
238+
// `examples/app-multi-package` when the ladder lands by itself.
239+
const findings = validateObjectReferences(perPackageStack(ORDERS_BODY, undefined));
240+
expect(findings).toHaveLength(1);
241+
expect(findings[0].severity).toBe('error');
242+
expect(findings[0].rule).toBe(OBJECT_REFERENCE_UNKNOWN);
243+
expect(findings[0].path).toBe('objects[0].fields.account.reference');
244+
expect(findings[0].message).toContain('lookup target "crm_account"');
245+
});
246+
247+
it('resolves a lookup into an object a SIBLING package of the same artifact provides', () => {
248+
const artifact = [{ manifest: ORDERS_BODY }, { manifest: CORE_BODY }];
249+
expect(validateObjectReferences(perPackageStack(ORDERS_BODY, artifact))).toEqual([]);
250+
// …and the sibling, judged from its own side, is unaffected.
251+
expect(validateObjectReferences(perPackageStack(CORE_BODY, artifact))).toEqual([]);
252+
});
253+
254+
it('NON-DEGENERACY — a name NO package in the artifact provides still errors', () => {
255+
// The whole distinction between the ruled fix and "skip the field site per
256+
// package": the site is still judged, against a wider set. `crm_contract`
257+
// is the card's cross-REPO spelling — no entry here ships it.
258+
const dangling = {
259+
...ORDERS_BODY,
260+
objects: [
261+
{ name: 'crm_order', fields: { contract: { type: 'lookup', reference: 'crm_contract' } } },
262+
],
263+
};
264+
const findings = validateObjectReferences(
265+
perPackageStack(dangling, [{ manifest: dangling }, { manifest: CORE_BODY }]),
266+
);
267+
expect(findings).toHaveLength(1);
268+
expect(findings[0].severity).toBe('error');
269+
expect(findings[0].rule).toBe(OBJECT_REFERENCE_UNKNOWN);
270+
expect(findings[0].path).toBe('objects[0].fields.contract.reference');
271+
// The remedy lists what the ARTIFACT provides, not only this package's own
272+
// objects — that is the list the author actually has to choose from.
273+
expect(findings[0].hint).toContain('Defined objects: crm_account, crm_order.');
274+
});
275+
276+
it('carries the context to every site on the rule, not only the field one', () => {
277+
const withNav = {
278+
...ORDERS_BODY,
279+
navigation: [{ name: 'accounts', objectName: 'crm_account', requiresObject: 'crm_account' }],
280+
actions: [{ name: 'link', params: [{ name: 'a', type: 'lookup', reference: 'crm_account' }] }],
281+
};
282+
expect(
283+
validateObjectReferences(
284+
perPackageStack(withNav, [{ manifest: withNav }, { manifest: CORE_BODY }]),
285+
),
286+
).toEqual([]);
287+
});
288+
289+
it('reads only names a package REALLY declares — an entry with no body contributes none', () => {
290+
// A future `{ ref, integrity }` external segment carries no manifest
291+
// content (ADR-0130 D4). Inventing a name for such an entry would silence
292+
// the ladder, which is the one mistake this context must not make.
293+
const findings = validateObjectReferences(
294+
perPackageStack(ORDERS_BODY, [{ ref: 'sha256-x' }, { manifest: { id: 'x' } }, 'not-an-entry']),
295+
);
296+
expect(findings).toHaveLength(1);
297+
expect(findings[0].path).toBe('objects[0].fields.account.reference');
298+
});
299+
300+
it('ignores a `packages` value that is not a list of entries', () => {
301+
for (const packages of [null, 42, 'core']) {
302+
const findings = validateObjectReferences(perPackageStack(ORDERS_BODY, packages));
303+
expect(findings.map((f) => f.path)).toEqual(['objects[0].fields.account.reference']);
304+
}
305+
});
306+
});
307+
83308
describe('validateObjectReferences — dashboard global filters', () => {
84309
// The other HotCRM `object: 'user'` instance.
85310
it('errors on optionsFrom.object naming a nonexistent object', () => {

0 commit comments

Comments
 (0)