From eb458b65eaad760ca6ac9dc481d256bda4268d62 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:42:17 +0800 Subject: [PATCH] =?UTF-8?q?fix(lint,cli):=20os=20lint=20=E4=B8=8D=E5=86=8D?= =?UTF-8?q?=E6=94=BE=E8=A1=8C=E5=8F=A6=E5=A4=96=E4=B8=A4=E4=B8=AA=E5=91=BD?= =?UTF-8?q?=E4=BB=A4=E6=8B=92=E7=BB=9D=E7=9A=84=20flow=20(#4345=20?= =?UTF-8?q?=E5=90=8E=E7=BB=AD)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateReadonlyFlowWrites` 手工接进了 `os validate` 和 `os compile`,唯独 漏了 `os lint`。在 showcase 上植入一处违规(`runAs:'user'` 的 update_record 写静态 readonly 字段)实测: os lint os validate 修复前 exit 0 放行 exit 1 拒绝 修复后 exit 1 拒绝 exit 1 拒绝 这条规则是 **gate** 级(静态 readonly + 字面字段是确定的 no-op:引擎从 UPDATE payload 里剥掉它,步骤照样报成功,#2948/#3425),所以漏接不是少一条 警告 —— 而是 `os lint` 放行了一个 `os validate` 会拦下的构建。 现接入 `REFERENCE_INTEGRITY_RULES`,两处手工接线随之删除,三个命令的答案 由构造保证一致,而不是靠三个人记得。这正是 suite 设立要终结的漂移 (#3583 §5 D5),而 suite 自己的头注释一直把这条规则当作该漂移的现行证据。 两件事让这个接线从"不整洁"变成"站不住": - `validateFlowNodeWrites`(#4369)走的是**同一张** `config.fields` map,问的 是同一问题的另一半("字段存在吗" vs "字段可写吗"),而它已经在 suite 里。 一张 map、两个检查、两套命令集合。 - 两处手工接线连喂进去的输入都不一致:`validate` 喂 pre-parse 的 `normalized`,`compile` 喂 post-parse 的 `result.data`。合并到 suite 的 post-parse 输入前已实测两者对本规则等价,不丢 finding。 规则行为零变化:id、严重级、文案都不动。 Co-Authored-By: Claude Opus 5 --- .../readonly-flow-writes-joins-the-suite.md | 38 +++++++++++++++++++ packages/cli/src/commands/compile.ts | 34 ----------------- packages/cli/src/commands/validate.ts | 38 +------------------ .../src/reference-integrity-suite.test.ts | 28 ++++++++++++-- .../lint/src/reference-integrity-suite.ts | 15 +++++++- 5 files changed, 77 insertions(+), 76 deletions(-) create mode 100644 .changeset/readonly-flow-writes-joins-the-suite.md 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 }, ]; /**