fix(lint): stop the readonlyWhen hints ruling out the working remedy and offering a useless one (#14202)

Message text only across three carriers; rule ids, severities and match sets
untouched.

The flow hint recommended runAs:'system'. The conditional strip has no
isSystem guard at all, so that is a privilege widening for no behaviour
change (LOCK 2 pins it). The hook hint and the hook-bodies.mdx bullet
asserted readonlyWhen strips a beforeUpdate-derived value -- the behaviour
#9107 removed -- thereby ruling out the one remedy that works.

All three now name the two measured remedies and refuse elevation, following
the shape action-api-update-readonly-when-field already ships. The
static-readonly hints that recommend elevation are deliberately unchanged;
a new pin holds the two apart.

Fixes #13832


Claude-Session: https://claude.ai/code/session_01WLJQhde67SeTccsmnBVarV

Co-authored-by: Claude <noreply@anthropic.com>

Author: baozhoutao

.changeset/readonly-when-hints-drop-elevation.md

@@ -0,0 +1,52 @@
+---
+"@objectstack/lint": patch
+---
+
+fix(lint): stop the `readonlyWhen` hints ruling out the remedy that works and offering one that does not (#13832)
+
+Message text only. Rule ids, severities and match sets are untouched, and no
+finding changes shape — but the hint **is** the whole product of an advisory
+rule (neither finding blocks a build), so the sentence is all the author acts
+on, and both of these sentences were measured false against the engine.
+
+`flow-update-readonly-flow-writes`' `flow-update-readonly-when-field` hint said:
+
+> If automation must maintain this field regardless of record state, run the flow runAs:'system'.
+
+It does not. The conditional strip has **no `isSystem` guard at all** —
+`stripReadonlyWhenFields` runs unconditionally on the update path, unlike the
+static `readonly` strip beside it that really is skipped for system callers.
+So the advice bought the author a `runAs:'system'` flow, a re-run, the same
+missing column, and an elevated run identity in the tree with no compensating
+behaviour: **a privilege widening for no effect**. Pinned as "LOCK 2 — isSystem
+does NOT exempt a caller-supplied value" in
+`engine-readonly-when-derived-writes.test.ts`, and from the strict-mode side as
+"covers readonlyWhen too — the arm a trusted (isSystem) caller can still hit".
+
+Both the `hook-api-update-readonly-when-field` hint and the matching
+`content/docs/automation/hook-bodies.mdx` bullet carried the same defect from
+the other direction — they **ruled out the remedy that works**:
+
+> readonlyWhen strips even a beforeUpdate-derived value, so an own-hook stamp is NOT a workaround here
+
+That is the behaviour #9107 removed. The conditional strip now judges the
+*caller's* entry snapshot, so a value a `beforeUpdate` hook **derives** is not
+caller-supplied and lands even on a locked record —
+`engine-readonly-when-derived-writes.test.ts` opens with "THE REPORT: a
+hook-derived value on a TRUE readonlyWhen field now LANDS", and pins the bulk
+path on the same terms. Between them the two halves left the author's only
+working option struck out and a useless one recommended.
+
+All three hints now name the same two measured remedies — confirm the write
+only targets records whose predicate is FALSE, or derive the field in a
+`beforeUpdate` hook on the target object — and refuse elevation explicitly,
+matching the shape `action-api-update-readonly-when-field` already shipped.
+The hook hint keeps its stronger, separate reason that `sudo()` is a
+`TypeError` from a sandboxed body, and now also carries the reason that
+survives if that one is ever fixed: a system context does not waive the
+conditional lock either.
+
+Deliberately **not** flattened: the static-`readonly` hints and docs rows that
+recommend elevation stay exactly as they are, because for *that* strip
+elevation is the intended channel. The two disagree for a reason, and a pin now
+holds them apart.

content/docs/automation/hook-bodies.mdx

@@ -264,7 +264,7 @@ There is an asymmetry here that costs data if you learn it the hard way, so lear
 The dropped case is the dangerous one: nothing fails, the step reports success, and the column is simply always null. Because both halves of that judgement are declared in your own stack, it is checked at author time and **gates the build**:
 
 - `hook-api-update-readonly-field` — **error**. A body's literal `ctx.api.object('…').update()` / `.updateById()` writes a field the named object declares `readonly: true`.
-- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. Note that `readonlyWhen` also strips a `beforeUpdate`-derived value, so the own-hook stamp is **not** a workaround for it — and neither is `sudo()`, which a body cannot reach (see the row above). On this shape, confirm the write only targets records whose predicate is `false`, or drop the field from the payload.
+- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. The own-hook stamp **is** the workaround here, exactly as it is for static `readonly`: since [#9107](https://github.com/objectstack-ai/objectstack/issues/9107) the conditional strip judges the *caller's* entry payload, so a value a `beforeUpdate` hook **derives** is not caller-supplied and lands even on a locked record. (Deriving is the operative word — a hook that merely echoes the caller's own value back has written nothing the strip can tell from the caller's, and it still goes.) What does **not** help is elevation: `sudo()` a body cannot reach (see the row above), and — unlike the static strip — the conditional lock is **not** waived by a system context either, so there is no elevated caller for which a caller-supplied value survives. On this shape, confirm the write only targets records whose predicate is `false`, or derive the field in a `beforeUpdate` hook on the target object.
 
 Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name, an object this stack does not declare, and `insert`/`create` are all skipped, so the rule has no opinion on them. The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425).
 

packages/lint/src/validate-readonly-flow-writes.test.ts

@@ -146,6 +146,46 @@ describe('validateReadonlyFlowWrites', () => {
     expect(findings[0].message).toContain('#3042');
   });
 
+  // The hint is the WHOLE product of an advisory rule - the finding blocks
+  // nothing, so the sentence is all the author acts on. This one used to read
+  // "run the flow runAs:'system'", which is advice to widen a write's
+  // privileges for NO behaviour change: the conditional strip has no `isSystem`
+  // guard at all, so the elevated run drops the field on a locked record
+  // exactly as the user run does.
+  it('does NOT offer elevation as the remedy — runAs:system does not waive the conditional lock', () => {
+    const [finding] = validateReadonlyFlowWrites({
+      objects: [opportunityObject],
+      flows: [flowWith({ amount: 5000 }, { runAs: 'user' })],
+    });
+
+    // The refusal, and the reason that makes it checkable rather than a slogan.
+    // Pinned against "LOCK 2 - isSystem does NOT exempt a caller-supplied value"
+    // in `engine-readonly-when-derived-writes.test.ts`, and the strict-mode
+    // sibling's "covers readonlyWhen too - the arm a trusted (isSystem) caller
+    // can still hit".
+    expect(finding.hint).toContain('Elevation is not a workaround here');
+    expect(finding.hint).toContain('NOT waived by a system context');
+    expect(finding.hint).not.toMatch(/run the flow runAs:'system'/);
+
+    // The two remedies that DO work, both named — the same pair the action and
+    // hook siblings offer. (2) is #9107: the strip judges the CALLER's entry
+    // payload, so a hook-derived value lands even on a locked record ("THE
+    // REPORT: a hook-derived value on a TRUE readonlyWhen field now LANDS").
+    expect(finding.hint).toContain('readonlyWhen predicate is FALSE');
+    expect(finding.hint).toContain('beforeUpdate hook');
+    expect(finding.hint).toContain('does land, even on a locked record');
+
+    // The static-`readonly` sibling hint keeps recommending runAs:'system',
+    // because for THAT strip elevation really is the intended channel. The two
+    // disagree for a reason; pinned here so a future sweep cannot flatten them.
+    const staticFinding = validateReadonlyFlowWrites({
+      objects: [opportunityObject],
+      flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' })],
+    })[0];
+    expect(staticFinding.rule).toBe(FLOW_UPDATE_READONLY_FIELD);
+    expect(staticFinding.hint).toContain("runAs:'system'");
+  });
+
   it('separates readonly (error) + readonlyWhen (warning) + plain (clean) in one node', () => {
     const findings = validateReadonlyFlowWrites({
       objects: [opportunityObject],

packages/lint/src/validate-readonly-flow-writes.ts

@@ -18,9 +18,20 @@
 //     field is NOT a no-op and is never flagged.
 //
 //   • Only `runAs !== 'system'`. A `runAs:'system'` run is elevated and the
-//     engine skips the strip entirely, so a system flow legitimately MAINTAINS
-//     readonly fields ("users can't edit this, but automation does"). That is
-//     the intended channel, so it is never flagged.
+//     engine skips the STATIC `readonly` strip, so a system flow legitimately
+//     MAINTAINS readonly fields ("users can't edit this, but automation does").
+//     That is the intended channel, so it is never flagged.
+//
+//     ⚠️ That exemption is the STATIC strip's alone. `stripReadonlyWhenFields`
+//     runs with no `isSystem` guard at all (engine.ts, the #9107 note: "`isSystem`
+//     is still NOT an exemption here, unlike the static strip below"), pinned as
+//     "LOCK 2 - isSystem does NOT exempt a caller-supplied value" in
+//     `engine-readonly-when-derived-writes.test.ts`. So elevation is NOT a
+//     `readonlyWhen` remedy, and this rule's hint must never offer it. The skip
+//     above is therefore WIDER than the conditional lock warrants - a
+//     `runAs:'system'` flow writing a `readonlyWhen` field is still stripped on a
+//     locked record and goes unflagged. Left as-is deliberately: the match set is
+//     out of scope for the message-text correction that fixed the hint.
 //
 //   • Static `readonly:true` + a LITERAL field name is a 100%-certain no-op →
 //     ERROR (gates the build). `readonlyWhen` is per-record-state — it strips
@@ -204,8 +215,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind
               `where that predicate is TRUE, a runAs:'${runAs}' UPDATE strips the field (#3042), so this ` +
               `write may silently not land depending on the record's state.`,
             hint:
-              `If automation must maintain this field regardless of record state, run the flow runAs:'system'. ` +
-              `Otherwise confirm this node only targets records whose readonlyWhen predicate is FALSE.`,
+              `Elevation is not a workaround here: unlike the static readonly strip, the conditional lock ` +
+              `is NOT waived by a system context, so runAs:'system' strips this field on a locked record ` +
+              `exactly as this run does. Either confirm this node only targets records whose readonlyWhen ` +
+              `predicate is FALSE, or derive '${fieldName}' in a beforeUpdate hook on '${objectName}' - a ` +
+              `hook-derived value is not caller-supplied and does land, even on a locked record. Otherwise ` +
+              `remove '${fieldName}' from this update_record node. This warning never blocks a build.`,
           });
         }
       }

packages/lint/src/validate-readonly-hook-writes.test.ts

@@ -337,23 +337,48 @@ describe('validateReadonlyHookWrites - GREEN: nothing statically knowable is gue
 });
 
 describe('validateReadonlyHookWrites - readonlyWhen is a SECOND shape, not the same verdict', () => {
-  // #9107: readonlyWhen strips per record STATE, and it strips a
-  // beforeUpdate-derived value too. So the write is conditional, not certain -
-  // warning, exactly as the flow sibling grades it.
+  // readonlyWhen strips per record STATE, so the write is conditional, not
+  // certain - warning, exactly as the flow sibling grades it.
   it('grades a readonlyWhen field as an advisory warning, not an error', () => {
     const findings = validateReadonlyHookWrites(
       crmStack("await ctx.api.object('crm_account').update({ credit_hold: true });"),
     );
     expect(findings).toHaveLength(1);
     expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_WHEN_FIELD);
     expect(findings[0].severity).toBe('warning');
-    // The own-hook stamp is NOT the remedy here, and the hint must not offer it.
-    expect(findings[0].hint).not.toContain('ctx.input.credit_hold');
-    // [#14010] Nor is sudo, for the sandbox-reachability reason above - so this
-    // hint offers NEITHER, and says which record states the write is safe on.
-    expect(findings[0].hint).toContain('not marshalled into the sandbox');
-    expect(findings[0].hint).not.toMatch(/write it through ctx\.api\.sudo/);
-    expect(findings[0].hint).toContain('readonlyWhen predicate is FALSE');
+  });
+
+  // The hint is the WHOLE product of an advisory rule - the finding blocks
+  // nothing, so the sentence is all the author acts on. Both remedies it names
+  // are pinned against the engine, and the one it refuses to name is the one
+  // that would cost the author a privilege widening for no behaviour change.
+  it('offers the beforeUpdate-derived stamp as the remedy, and does NOT offer elevation', () => {
+    const [finding] = validateReadonlyHookWrites(
+      crmStack("await ctx.api.object('crm_account').update({ credit_hold: true });"),
+    );
+
+    // Remedy (1): the record states on which the write is safe.
+    expect(finding.hint).toContain('readonlyWhen predicate is FALSE');
+
+    // Remedy (2). #9107 made the conditional strip judge the CALLER's entry
+    // snapshot, so a hook-DERIVED value survives on a locked record - pinned in
+    // `engine-readonly-when-derived-writes.test.ts` as "THE REPORT: a
+    // hook-derived value on a TRUE readonlyWhen field now LANDS". The hint used
+    // to assert the opposite and thereby rule out the one remedy that works.
+    expect(finding.hint).toContain('beforeUpdate hook');
+    expect(finding.hint).toContain('does land, even on a locked record');
+    expect(finding.hint).not.toMatch(/strips even a beforeUpdate-derived value/);
+    expect(finding.hint).not.toMatch(/own-hook stamp is NOT a workaround/);
+
+    // NOT elevation, for two independent reasons, both stated. `sudo()` is
+    // unreachable from a body ([#14010] - QuickJS `ctx.api` carries no `sudo`),
+    // AND a system context does not waive the conditional lock anyway ("LOCK 2 -
+    // isSystem does NOT exempt a caller-supplied value"). The second reason is
+    // what makes this hint's refusal survive if the first is ever fixed.
+    expect(finding.hint).toContain('Elevation is not a workaround here');
+    expect(finding.hint).toContain('not marshalled into the sandbox');
+    expect(finding.hint).toContain('does not waive the conditional lock');
+    expect(finding.hint).not.toMatch(/write it through ctx\.api\.sudo/);
   });
 
   it('reports a field carrying BOTH flags as the certain (static readonly) finding', () => {

packages/lint/src/validate-readonly-hook-writes.ts

@@ -315,17 +315,20 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind
           rule: HOOK_API_UPDATE_READONLY_WHEN_FIELD,
           where,
           path,
-          // The conditional strip is #3042; that it also removes a
-          // beforeUpdate-derived value is #9107. Both ids stay in this comment.
+          // The conditional strip is #3042. #9107 REMOVED its one over-reach:
+          // the strip now judges the CALLER's entry snapshot, so a value a
+          // beforeUpdate hook derives is no longer deleted. Both ids stay here.
           message:
             `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` +
             `readonlyWhen. On records whose predicate is TRUE that UPDATE strips the field, so this ` +
             `write may silently not land depending on the record's state.`,
           hint:
-            `readonlyWhen strips even a beforeUpdate-derived value, so an own-hook stamp is NOT a ` +
-            `workaround here - and neither is ctx.api.sudo(), which is not marshalled into the sandbox ` +
-            `(calling it from a body is a TypeError at run time). Confirm this call only targets records ` +
-            `whose readonlyWhen predicate is FALSE, or drop '${w.field}' from this payload.`,
+            `Either confirm this call only targets records whose readonlyWhen predicate is FALSE, or ` +
+            `derive '${w.field}' in a beforeUpdate hook on '${objectName}' - a hook-derived value is not ` +
+            `caller-supplied and does land, even on a locked record. Elevation is not a workaround here: ` +
+            `ctx.api.sudo() is not marshalled into the sandbox (calling it from a body is a TypeError at ` +
+            `run time), and a system context does not waive the conditional lock in any case. Otherwise ` +
+            `drop '${w.field}' from this payload. This warning never blocks a build.`,
         });
       }
     }