Skip to content

Import dry run with runAutomations: true can now report required for a field a beforeInsert hook would have populated #6537

Closed

Description

@os-project-manager

Observation-class finding, recorded while implementing #4633 (PR #6532), so the trade it makes is on the record rather than discovered later.

What changed

Before PR #6532 the import runner's required pre-check was gated on !runAutomations, with this reason:

Skipped when automations run: a beforeInsert hook may populate a required field, so we defer to the engine's own validation rather than false-reject here.

The gate was not applied to the bound checks #3956 added — those ran on every dry run regardless — so the two families were already asymmetric.

PR #6532 routes the whole dry run through DataProtocol.validateData and applies no runAutomations gate. validateData runs no hooks (a documented, deliberate boundary of #6037: firing user-authored hooks inside a preview is the retired validateOnly defect respelled). So on an object whose beforeInsert hook derives a required business field, a dry run with automations on can now report required for a row the real write would create.

Why it was done this way

runAutomations has defaulted to true since #2922. Gating the validateData call on !runAutomations would leave the DEFAULT dry run with no validation at all — strictly worse than both the pre-#6532 state and the state #4633 set out to fix. The trade taken was: close the false all-clear on every dry run, and accept the narrow hook-derived false alarm that #6037 already documents as its boundary.

Scope of the exposure

Narrow, and worth measuring before acting:

  • audit and ownership stamps (created_by, owner_id, organization_id, …) are system/readonly and are skipped by validation anyway, so they cannot produce this;
  • what remains is a hook deriving a business field that its own object also declares required with no defaultValue.

No example of that shape was found in examples/app-crm or examples/app-showcase while implementing #6532, but the search was not exhaustive.

Possible dispositions

Recording, not recommending — the measurement above is what a triage round needs first.

Activity

  1. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Findings round — routing repair only: adding domain:cli; the finding grade is untouched and no ownership is taken.

    Landing: the documented trade lives in the packages/rest import path (dry run via PR #6532), and the cheapest recorded disposition — reporting hook-covered required rows through the warnings[] channel PR #6532 added — also lands in packages/rest → domain:cli by the table. Fork noted, not resolved: disposition 3 (a declared-safe derivation subset in validateData) would land in domain:engine-core; if the eventual verdict goes that way, re-route the execution card then — the label follows today's most probable landing, not a guess at the verdict.

    Premise note: PR #6532 is an open draft, so this card's "now" is conditional on that merge — correctly held as finding until the pool is graded after #6532 lands.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Findings round: held — finding stays, and here is why it stays rather than moving either direction.

    Stale-premise check: PR #6532 merged at 04:56Z, so the behavior this card describes is now the shipped behavior, not a prediction. The exposure is real but is the documented boundary of ruling D (#4633, maintainer 2026-08-06): validateData runs no hooks by #6037's deliberate design, and gating the dry-run's validation on !runAutomations would strictly regress the default path.

    Why not promote: the cheapest disposition (report hook-derived required as a warnings[] entry) reopens the verdict-vocabulary split #6037 explicitly avoided — that is a product-semantics fork, not a mechanical fix. Why not inbox: the card's own measurement found zero instances of the exposed shape (hook-derived required business field) in examples/app-crm / app-showcase, so the maintainer would be asked to rule on a failure nobody has hit.

    Restart condition: first real import report of a required false alarm traced to a beforeInsert-derived field, or any maintainer revisit of #6037's no-hooks boundary — either reopens this as a graded promote (likely to the inbox, since all three dispositions in the body are contract calls).

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions