diff --git a/.changeset/readonly-flow-writes-joins-the-suite.md b/.changeset/readonly-flow-writes-joins-the-suite.md new file mode 100644 index 0000000000..88b0174b34 --- /dev/null +++ b/.changeset/readonly-flow-writes-joins-the-suite.md @@ -0,0 +1,38 @@ +--- +"@objectstack/lint": patch +"@objectstack/cli": patch +--- + +fix(lint,cli): `os lint` no longer passes a flow the other two commands refuse + +`validateReadonlyFlowWrites` was hand-wired into `os validate` and `os compile` +and never into `os lint`. Measured on the showcase app with one planted +violation — a `runAs:'user'` `update_record` writing a static-`readonly` field: + +| | `os lint` | `os validate` | +|---|---|---| +| before | **exit 0 — passed** | exit 1 — refused | +| after | exit 1 — refused | exit 1 — refused | + +That rule **gates** (a static `readonly` + literal field is a certain no-op: +the engine strips it from the UPDATE payload while the step still reports +success, #2948/#3425), so the divergence was not a missing warning — `os lint` +green-lit a build `os validate` stops. + +It now joins `REFERENCE_INTEGRITY_RULES`, and both hand-wired call sites are +deleted with it, so the three commands share one answer by construction rather +than by three people remembering. This is the drift the suite was created to end +(#3583 §5 D5) and which its own header cited this rule as the standing proof of. + +Two things made the wiring indefensible rather than merely untidy: + +- `validateFlowNodeWrites` (#4369) walks the **same** `config.fields` map to ask + the other half of the question — "does this field exist?" against "is it + writable?" — and is already a suite member. One map, two checks, two different + command sets. +- The two hand-wired sites did not even agree with each other on their input: + `validate` passed the PRE-parse `normalized` stack, `compile` the POST-parse + `result.data`. Verified equivalent for this rule before collapsing them onto + the suite's post-parse input, so no finding is lost. + +No rule behaviour changes: same ids, same severities, same messages. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 6e82d11734..e6a7680dd2 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -23,7 +23,6 @@ import { validateFilterTokens } from '@objectstack/lint'; import { validateReferenceIntegrity } from '@objectstack/lint'; import { validateResponsiveStyles } from '@objectstack/lint'; import { validateSecurityPosture, validateOrgAxisRedLines, buildAccessMatrix, diffAccessMatrix } from '@objectstack/lint'; -import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { lintFlowPatterns } from '../utils/lint-flow-patterns.js'; import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js'; import { lintUniqueDeclarations } from '../lint/data-model-rules.js'; @@ -618,39 +617,6 @@ export default class Compile extends Command { } } - // 3e2. [#3425] Readonly flow-write guardrail. A `runAs:user` update_record - // writing a static-`readonly` field is a silent no-op — the engine - // strips it from the UPDATE payload (#2948) while the step reports - // success. This GATES the build (shift-left of the #3407/#3413 - // run-time strip warning); `readonlyWhen` writes are per-record-state, - // so they are advisory, printed dimmed and never fatal. - if (!flags.json) printStep('Checking readonly flow writes (#3425)...'); - const readonlyWriteFindings = validateReadonlyFlowWrites(result.data as Record); - const readonlyWriteErrors = readonlyWriteFindings.filter((f) => f.severity === 'error'); - const readonlyWriteAdvisories = readonlyWriteFindings.filter((f) => f.severity !== 'error'); - if (readonlyWriteErrors.length > 0) { - if (flags.json) { - await emitJson({ success: false, error: 'readonly flow-write validation failed', issues: readonlyWriteErrors }, 0, { compact: true }); - this.exit(1); - } - console.log(''); - printError(`Readonly flow-write check failed (${readonlyWriteErrors.length} issue${readonlyWriteErrors.length > 1 ? 's' : ''})`); - for (const f of readonlyWriteErrors.slice(0, 50)) { - console.log(` • ${f.where}: ${f.message}`); - console.log(chalk.dim(` ${f.hint}`)); - console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`)); - } - this.exit(1); - } - if (readonlyWriteAdvisories.length > 0 && !flags.json) { - console.log(''); - for (const f of readonlyWriteAdvisories) { - printWarning(`${f.where}: ${f.message}`); - console.log(chalk.dim(` ${f.hint}`)); - console.log(chalk.dim(` rule: ${f.rule}`)); - } - } - // 3f. [ADR-0090 D6] Access-matrix snapshot gate. Opt-in per app: when // `access-matrix.json` sits next to the config, the (permission set // × object) capability matrix derived from THIS build must match it diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index ec47e4d7c3..ae5d462beb 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -28,7 +28,6 @@ import { validateCapabilityReferences } from '@objectstack/lint'; import { validateVisibilityPredicates } from '@objectstack/lint'; import { validateSecurityPosture, validateOrgAxisRedLines } from '@objectstack/lint'; import { validateFlowTriggerReadiness } from '@objectstack/lint'; -import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { lintFlowPatterns } from '../utils/lint-flow-patterns.js'; import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js'; import { lintUniqueDeclarations } from '../lint/data-model-rules.js'; @@ -540,41 +539,6 @@ export default class Validate extends Command { // flow whose filter token the runtime refuses (#3810) for as long as // this call site was the only one. - // 3e2. [#3425] Readonly flow-write guardrail. A `runAs:user` update_record - // that writes a static-`readonly` field is a SILENT no-op — the engine - // strips it from the UPDATE payload (#2948) yet the step reports - // success. This is the shift-left of the run-time strip warning - // (#3407/#3413): a static readonly + literal field is a certain no-op - // → error (gates); a `readonlyWhen` field is per-record-state → advisory. - if (!flags.json) printStep('Checking readonly flow writes (#3425)...'); - const readonlyWriteFindings = validateReadonlyFlowWrites(normalized as Record); - const readonlyWriteErrors = readonlyWriteFindings.filter((f) => f.severity === 'error'); - const readonlyWriteWarnings = readonlyWriteFindings.filter((f) => f.severity === 'warning'); - if (readonlyWriteErrors.length > 0) { - if (flags.json) { - await emitJson({ - valid: false, - errors: readonlyWriteErrors, - duration: timer.elapsed(), - }); - this.exit(1); - } - console.log(''); - printError(`Readonly flow-write check failed (${readonlyWriteErrors.length} issue${readonlyWriteErrors.length > 1 ? 's' : ''})`); - for (const f of readonlyWriteErrors.slice(0, 50)) { - console.log(` • ${f.where}: ${f.message}`); - console.log(chalk.dim(` ${f.hint}`)); - console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`)); - } - this.exit(1); - } - if (!flags.json) { - for (const f of readonlyWriteWarnings.slice(0, 50)) { - console.log(chalk.yellow(` ⚠ ${f.where}: ${f.message}`)); - console.log(chalk.dim(` ${f.hint}`)); - } - } - // 3e3. [#3782] The four authoring lints that live in the CLI itself rather // than in `@objectstack/lint`. Every other gate on this command is a // `@objectstack/lint` import, so these four were only ever reachable @@ -749,7 +713,7 @@ export default class Validate extends Command { // the suite's warnings, though the failure path (above) and the console // both did. Same shape of bug as the dropped errors — computed, then // discarded — so it is fixed rather than reproduced under a new name. - warnings: [...exprWarnings, ...widgetWarnings, ...actionRefWarnings, ...styleWarnings, ...jsxWarnings, ...capWarnings, ...flowReadinessWarnings, ...refWarnings, ...readonlyWriteWarnings, ...authoringLintWarnings, ...unknownKeyWarnings, ...securityAdvisories, ...capProviderWarnings], + warnings: [...exprWarnings, ...widgetWarnings, ...actionRefWarnings, ...styleWarnings, ...jsxWarnings, ...capWarnings, ...flowReadinessWarnings, ...refWarnings, ...authoringLintWarnings, ...unknownKeyWarnings, ...securityAdvisories, ...capProviderWarnings], conversions: conversionNotices, specVersionGap: specGap, duration: timer.elapsed(), diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index 2428c302dc..3072c75959 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -29,6 +29,7 @@ describe('reference-integrity suite — membership', () => { 'validateHookBodyWrites', 'validateActionBodyWrites', 'validateFlowNodeWrites', + 'validateReadonlyFlowWrites', ]); }); @@ -51,7 +52,10 @@ describe('reference-integrity suite — every member actually runs', () => { objects: [ { name: 'crm_lead', - fields: { name: { type: 'text', label: 'Name' } }, + fields: { + name: { type: 'text', label: 'Name' }, + locked: { type: 'boolean', label: 'Locked', readonly: true }, + }, // validateSearchableFields: `budget` is not a field on crm_lead, so the // ADR-0061 declaration is stale — the engine drops it and searches a // narrower set than the object declares. @@ -157,6 +161,12 @@ describe('reference-integrity suite — every member actually runs', () => { { name: 'lead_followup', type: 'record_change', + // validateReadonlyFlowWrites: a runAs:'user' update_record writing a + // static-`readonly` field. The engine strips it and the step still + // reports success (#2948/#3425) — a certainty, so it GATES. Walks the + // same `config.fields` map validateFlowNodeWrites does, which is why + // the two must not diverge across commands again. + runAs: 'user', nodes: [ { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } }, // validateFlowTemplatePaths: `budget` is not a field on crm_lead. In a @@ -178,6 +188,17 @@ describe('reference-integrity suite — every member actually runs', () => { type: 'update_record', config: { objectName: 'crm_lead', fields: { lead_score: 100 } }, }, + // validateReadonlyFlowWrites: the OTHER question about that same + // `config.fields` map — `locked` exists but is static-`readonly`, so + // under this flow's `runAs:'user'` the engine strips it and the step + // still reports success (#2948/#3425). A separate node from `stamp` + // on purpose: one node carrying both defects would let either rule + // go silent behind the other's finding. + { + id: 'lock', + type: 'update_record', + config: { objectName: 'crm_lead', fields: { locked: true } }, + }, ], }, ], @@ -204,6 +225,7 @@ describe('reference-integrity suite — every member actually runs', () => { // why it rides along instead of becoming its own entry. expect(rules).toContain('action-record-write-discarded'); expect(rules).toContain('flow-node-write-unknown-field'); + expect(rules).toContain('flow-update-readonly-field'); }); it('carries a gating flow-template finding through the suite (#3810)', () => { @@ -225,9 +247,9 @@ describe('reference-integrity suite — every member actually runs', () => { expect(typeof f.message).toBe('string'); expect(typeof f.hint).toBe('string'); } - // Object references run first, flow-node writes last. + // Object references run first, readonly flow writes last. expect(findings[0].rule).toBe('object-reference-unknown'); - expect(findings[findings.length - 1].rule).toBe('flow-node-write-unknown-field'); + expect(findings[findings.length - 1].rule).toBe('flow-update-readonly-field'); }); it('returns nothing for an empty stack', () => { diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 7c56200342..49cb040d93 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -68,6 +68,7 @@ import { validateAiAgentAuthoring } from './validate-ai-agent-authoring.js'; import { validateHookBodyWrites } from './validate-hook-body-writes.js'; import { validateActionBodyWrites } from './validate-action-body-writes.js'; import { validateFlowNodeWrites } from './validate-flow-node-writes.js'; +import { validateReadonlyFlowWrites } from './validate-readonly-flow-writes.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -133,8 +134,8 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // anyway because it falls out of the SAME parse of the SAME source: a // separate member would parse every action body twice to say two things // about one walk, and hand-wiring it into the CLI instead is exactly the - // drift this suite exists to end (`validateReadonlyFlowWrites` is the - // standing proof — wired into `validate` and `compile`, never into `lint`). + // drift this suite exists to end — which `validateReadonlyFlowWrites` was + // the standing proof of, until it joined the suite below. { name: 'validateActionBodyWrites', run: validateActionBodyWrites }, // The third surface that writes a record field set: a flow `update_record` // node's `config.fields`. Same question as the two rules above, but the map @@ -143,6 +144,16 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // standing "prefer a flow node, it's checked" advice was the least true of // the three until it landed. { name: 'validateFlowNodeWrites', run: validateFlowNodeWrites }, + // The OTHER question about that same `config.fields` map: not "does this + // field exist?" but "is it writable?" — a `runAs:'user'` update_record + // writing a static-`readonly` field is stripped by the engine and the step + // still reports success (#2948/#3425). It walks the identical map the rule + // above walks, so the two splitting call sites was never defensible: hand- + // wired into `validate` and `compile` only, it left `os lint` PASSING a flow + // `os validate` refuses — and this one gates, so the divergence shipped a + // build the other command would have stopped. Joining the suite is the whole + // fix; the two hand-wired call sites are deleted with it (#4345 follow-up). + { name: 'validateReadonlyFlowWrites', run: validateReadonlyFlowWrites }, ]; /**