Feature ETP-4793: Persist field visibility, F23 validator rule

.github/workflows/publish-preview.yml

@@ -1,12 +1,15 @@
 name: Publish preview packages
 
 # Publishes a throwaway PREVIEW (prerelease) of ALL publishable packages, in
-# lockstep, only when the feature branch has an open PR — so the
-# published-package path can be tested without cutting a real `latest` release.
+# lockstep, on every push to a feature branch — so the published-package path
+# can be tested without cutting a real `latest` release.
 #
 # See docs/plans/2026-07-14-preview-package-publishing.md (in etendo_schema_forge).
 #
-#   - Trigger: push to feature/** (auto) + manual workflow_dispatch, gated by PR.
+#   - Trigger: push to feature/** (auto) + manual workflow_dispatch. NOT gated
+#     by an open PR: the consumer is `make bump-core-version`, which needs only
+#     the published version, and a branch is often worth previewing before it is
+#     ready for review. An open PR only adds the sticky comment.
 #   - Version: <base>-preview.<branchid>.<timestamp>.<shortsha>.            (D3)
 #   - dist-tag: alpha — never `latest`, so consumers are unaffected.        (D2)
 #   - Scope: the 6 publishable packages, lockstep.                         (D6)
@@ -31,8 +34,10 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
+  # Looks up the branch's open PR, if any. This does NOT gate publishing — its
+  # only consumer is the sticky-comment step, which needs somewhere to post.
   find-pr:
-    name: Check for an open feature PR
+    name: Look up the branch's PR (optional)
     runs-on: ubuntu-latest
     outputs:
       number: ${{ steps.pr.outputs.number }}
@@ -47,14 +52,15 @@ jobs:
                  --json number --jq '.[0].number // empty')
           echo "number=$PR" >> "$GITHUB_OUTPUT"
           if [ -z "$PR" ]; then
-            echo "No open PR for $BRANCH — preview publication is skipped."
+            echo "No open PR for $BRANCH — publishing anyway, without a PR comment."
+            echo "Read the version off the 'preview-package' commit status or this run's log."
           else
             echo "Open PR #$PR found for $BRANCH."
           fi
 
   preview:
+    # `needs` only to read the PR number for the comment step — never to gate.
     needs: find-pr
-    if: needs.find-pr.outputs.number != ''
     name: Build, test & publish preview
     runs-on: ubuntu-latest
     steps:
@@ -132,13 +138,14 @@ jobs:
       # --- Make the published version "visible fácil" -----------------------
       # A) Commit status: pins "alpha: <version>" to this SHA, so it shows as a
       #    check on the commit AND in the PR's checks list — no need to open the
-      #    run to see what was published. Links back to this run.
-      # B) PR comment: post a single fresh comment on the PR that gated publishing.
+      #    run to see what was published. Links back to this run. This is the
+      #    ONLY surface that always exists, so it carries the no-PR case.
+      # B) PR comment: when the branch has an open PR, post a single fresh comment
       #    with the version and the exact pin snippet. On each push the previous
       #    bot comment (matched by marker) is deleted and a NEW one is posted, so
       #    it always lands at the bottom of the PR and notifies subscribers
-      #    (editing in place would leave it buried and silent). Skipped silently
-      #    when there is no open PR (the find-pr job skips this entire job).
+      #    (editing in place would leave it buried and silent). Skipped when there
+      #    is no PR yet — publishing still happened; read the version off (A).
       # Both are fail-soft: a comment/status hiccup must never fail the publish.
       - name: Pin published version to the commit (status)
         continue-on-error: true
@@ -153,6 +160,7 @@ jobs:
             -f target_url="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"
 
       - name: Comment published version on the open PR (sticky)
+        if: needs.find-pr.outputs.number != ''
         continue-on-error: true
         env:
           GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

cli/src/lib/entity-methods.js

@@ -269,6 +269,50 @@ export function applyMethodsToCrudPrediction(crud, methods, opts = {}) {
   return crud;
 }
 
+/**
+ * Is this AD tab's entity absent from the contract — i.e. did a human write
+ * `entities.<key>.exclude: true` in `decisions.json`?
+ *
+ * ETP-4793 — `exclude: true` had exactly one consumer, `resolve-curated.js`
+ * (`if (entityDecision.exclude === true) continue;`), so its whole effect was
+ * "absent from `contract.json`". Neither write path acted on it:
+ * `populateWindowSpec` walks `AD_Tab`, not the contract, so the entity still got
+ * an `ETGO_SF_ENTITY` row plus one `ETGO_SF_FIELD` row per AD column at the
+ * populate-step default `ISINCLUDED = 'Y'`, and nothing downstream closed them —
+ * `stepExcludeNonContractFields` compares column names against a FLAT set
+ * gathered across all contract entities, so an excluded entity's column survives
+ * whenever a contract entity happens to have a column of the same name.
+ *
+ * The measured consequence on the reference instance: 102 entities marked
+ * `exclude: true` across 28 specs, 90 of them present in `ETGO_SF_ENTITY`, 386
+ * of their field rows still `ISINCLUDED = 'Y'` — and one of them (`taxZone`)
+ * served with MORE verbs than the curated `tax` entity beside it, because a tab
+ * with no contract entity falls through to the window-level default while the
+ * curated entity is restricted by its own contract.
+ *
+ * Both write paths now consult this predicate and write `ISINCLUDED = 'N'` on
+ * the entity row, which is the lever every reader already honours: 24 call sites
+ * across the REST and MCP surfaces filter on `SFEntity.PROPERTY_ISINCLUDED`
+ * before touching an entity (`findIncludedEntity`, `listIncludedEntities`,
+ * `NeoServlet`, `NeoDiscoveryHelper`, …), and entity resolution always precedes
+ * field access. No new column, and no AD records, were needed.
+ *
+ * The empty-set guard is load-bearing: a contract with no `backendContract`
+ * entities at all (an older or partially-generated artifact) must answer `false`
+ * for every entity, or a single malformed contract would close an entire window.
+ *
+ * @param {object|null|undefined} contract - parsed contract.json
+ * @param {string} entityName - the NAME written to the ETGO_SF_ENTITY row
+ * @returns {boolean} true when the entity must be closed on both surfaces
+ */
+export function isEntityExcludedFromContract(contract, entityName) {
+  const entities = contract?.backendContract?.entities;
+  if (!entities || !entityName) return false;
+  const names = Object.keys(entities);
+  if (names.length === 0) return false;
+  return !names.includes(entityName);
+}
+
 /**
  * Read the resolved method list for an entity back off a generated contract.
  * This is the ONLY function the two write paths (`push-to-neo.js` and
@@ -278,7 +322,11 @@ export function applyMethodsToCrudPrediction(crud, methods, opts = {}) {
  *   1. `apiPrediction.crud.<entityName>.methods` — the resolved allowlist
  *   2. `apiPrediction.window.readOnly` — window-level default, which also covers
  *      AD tabs that have no contract entity at all (excluded entities still get
- *      an ETGO_SF_ENTITY row from `populateWindowSpec`)
+ *      an ETGO_SF_ENTITY row from `populateWindowSpec`). Those rows now carry
+ *      `ISINCLUDED = 'N'` — see `isEntityExcludedFromContract()` — so the method
+ *      flags resolved here are unreachable for them, and are deliberately left
+ *      at the window default rather than zeroed: an all-`N` set would break the
+ *      GET/GETBYID invariant this file enforces.
  *   3. all six methods — pre-ETP-4254 behaviour, unchanged
  *
  * ETP-4745 — `crud.<entityName>.delete === false` (set by `window.hideDelete` /

cli/src/lib/field-visibility.js

@@ -0,0 +1,96 @@
+/**
+ * field-visibility.js — The single source of truth for the curated-visibility →
+ * NEO-flag projection.
+ *
+ * `ETGO_SF_FIELD` stores the same decision twice, on purpose:
+ *   - `ISINCLUDED` / `ISREADONLY` — the two booleans NEO's runtime enforces.
+ *   - `VISIBILITY`               — the curated value verbatim, which `neo_schema`
+ *                                  hands to agents (`system` and `readOnly` both
+ *                                  collapse to Y/Y, so the flags cannot recover it).
+ *
+ * Storing a decision twice means it can disagree with itself. ETP-4793 / IMP-26
+ * found exactly that: `populateSpec` writes the two flags unconditionally but
+ * `VISIBILITY` only on some paths, so rows accumulate open flags with no
+ * curated value. `visibilityMatchesFlags()` is the predicate that detects it,
+ * and validator rule F23 is what runs it.
+ *
+ * Before ETP-4793 this function existed twice — exported from `push-to-neo.js`
+ * and inlined into `lib/neo-delta.js` to dodge a circular import. Both now
+ * import it from here. Do not add a third copy; a validator that re-implements
+ * the projection cannot detect a drift in the projection.
+ */
+
+/**
+ * Map a curated field visibility value to the NEO flag pair.
+ *
+ * Any value outside the curated set — including `null`, `undefined` and the
+ * empty string — is treated as `discarded` (closed). That default is what makes
+ * an absent `VISIBILITY` column coherent with `N`/`N` rather than a violation.
+ *
+ * @param {string|null|undefined} visibility - `editable` | `readOnly` | `system` | `discarded`
+ * @returns {{ isIncluded: 'Y'|'N', isReadOnly: 'Y'|'N' }}
+ */
+export function mapVisibility(visibility) {
+  switch (visibility) {
+    case 'editable':
+      return { isIncluded: 'Y', isReadOnly: 'N' };
+    case 'readOnly':
+      return { isIncluded: 'Y', isReadOnly: 'Y' };
+    case 'system':
+      return { isIncluded: 'Y', isReadOnly: 'Y' };
+    case 'discarded':
+      return { isIncluded: 'N', isReadOnly: 'N' };
+    default:
+      return { isIncluded: 'N', isReadOnly: 'N' };
+  }
+}
+
+/**
+ * The four curated visibility values, in the order the docs present them.
+ */
+export const CURATED_VISIBILITIES = Object.freeze(['editable', 'readOnly', 'system', 'discarded']);
+
+/**
+ * True when `visibility` is one of the four curated values. An absent or empty
+ * value is NOT curated — it is the "never written" state F23 reports separately
+ * from a genuine contradiction.
+ *
+ * @param {string|null|undefined} visibility
+ * @returns {boolean}
+ */
+export function isCuratedVisibility(visibility) {
+  return CURATED_VISIBILITIES.includes(visibility);
+}
+
+/**
+ * Check one stored row against the projection.
+ *
+ * Returns a verdict rather than a boolean because the two failure modes need
+ * different treatment (see F23 in docs/pipeline-validator-reference.md):
+ *
+ *   - `contradiction` — `VISIBILITY` holds a curated value whose projection
+ *     disagrees with the stored flags. Only a writer bug or a hand-edit can
+ *     produce this, so it blocks.
+ *   - `unwritten` — `VISIBILITY` is absent/empty while the flags say the field
+ *     is included. Coherent for a closed field (`N`/`N` IS the default
+ *     projection), a backfill gap for an open one, so it warns.
+ *
+ * @param {{ visibility?: string|null, isIncluded?: string, isReadOnly?: string }} row
+ * @returns {{ ok: boolean, kind: 'ok'|'contradiction'|'unwritten',
+ *             expected: { isIncluded: string, isReadOnly: string } }}
+ */
+export function visibilityMatchesFlags(row) {
+  const visibility = row?.visibility ?? null;
+  // Absent flags default to the closed pair, matching mapVisibility's default —
+  // a row that omits both columns is coherent with an omitted VISIBILITY.
+  const isIncluded = row?.isIncluded ?? 'N';
+  const isReadOnly = row?.isReadOnly ?? 'N';
+  const expected = mapVisibility(visibility);
+  const ok = expected.isIncluded === isIncluded && expected.isReadOnly === isReadOnly;
+  if (ok) return { ok: true, kind: 'ok', expected };
+  return {
+    ok: false,
+    kind: isCuratedVisibility(visibility) ? 'contradiction' : 'unwritten',
+    expected,
+  };
+}

cli/src/lib/neo-delta.js

@@ -39,23 +39,16 @@
 
 import { newEtendoId } from './etendo-uuid.js';
 import { indexByNaturalKey } from './etgo-xml-parser.js';
-import { methodsToXmlFlags, resolveContractEntityMethods } from './entity-methods.js';
-
-/**
- * Local copy of mapVisibility() from push-to-neo.js. Inlined to keep this
- * module free of circular imports (push-to-neo imports computeWindowDelta).
- * If you change one, mirror the change in the other — both are intentionally
- * tiny so divergence is easy to spot.
- */
-function mapVisibility(visibility) {
-  switch (visibility) {
-    case 'editable':  return { isIncluded: 'Y', isReadOnly: 'N' };
-    case 'readOnly':  return { isIncluded: 'Y', isReadOnly: 'Y' };
-    case 'system':    return { isIncluded: 'Y', isReadOnly: 'Y' };
-    case 'discarded': return { isIncluded: 'N', isReadOnly: 'N' };
-    default:          return { isIncluded: 'N', isReadOnly: 'N' };
-  }
-}
+import {
+  isEntityExcludedFromContract,
+  methodsToXmlFlags,
+  resolveContractEntityMethods,
+} from './entity-methods.js';
+// ETP-4793 — was an inlined copy (to dodge the push-to-neo → neo-delta import
+// cycle). Now a sibling in lib/, so there is no cycle and no second copy: the
+// live push, this offline projection and validator rule F23 all read the same
+// function.
+import { mapVisibility } from './field-visibility.js';
 
 function normalizeAgentPrompt(value) {
   if (value == null) return null;
@@ -65,8 +58,10 @@ function normalizeAgentPrompt(value) {
 
 /**
  * Local copy of normalizePreconditions() from push-to-neo.js. Inlined for the
- * same no-circular-import reason as mapVisibility/normalizeAgentPrompt — mirror
- * any change in the other. An explicit but empty declaration collapses to null
+ * same no-circular-import reason as normalizeAgentPrompt — mirror any change in
+ * the other. (mapVisibility used to be on this list; ETP-4793 moved it to
+ * lib/field-visibility.js instead, which is the better fix when a helper is
+ * needed by a third caller.) An explicit but empty declaration collapses to null
  * so a stale DB value gets cleared (ETP-4275).
  */
 function normalizePreconditions(value) {
@@ -249,6 +244,10 @@ export function computeWindowDelta(args) {
   // Map from naturalKey → spec/entity/field UUID (for deterministic FKs).
   const entityIdByNK = new Map();
 
+  // ETP-4793 — entity UUIDs whose contract entity is absent (`exclude: true`).
+  // Their field rows are closed too, mirroring `populateWindowSpec`.
+  const closedEntityIds = new Set();
+
   let entitySeq = 0;
   for (const tab of adTabs) {
     entitySeq++;
@@ -273,6 +272,12 @@ export function computeWindowDelta(args) {
       || desiredEntityByTabName.get(tab.name)
       || tab.name;
 
+    // ETP-4793 — mirror the live push: a tab the contract does not declare
+    // (`exclude: true`) is closed on the entity row. The live counterpart is
+    // `buildEntityMethodFlagsResolver` → `upsertEntity({ isIncluded: 'N' })`.
+    const entityExcluded = isEntityExcludedFromContract(contract, entityName);
+    if (entityExcluded) closedEntityIds.add(entityId);
+
     const entityRow = {
       _naturalKey: entityNK,
       ETGO_SF_ENTITY_ID: entityId,
@@ -281,7 +286,7 @@ export function computeWindowDelta(args) {
       AD_MODULE_ID: moduleId,
       NAME: entityName,
       SEQNO: String(entitySeq * 10),
-      ISINCLUDED: 'Y',
+      ISINCLUDED: entityExcluded ? 'N' : 'Y',
       // ETP-4254 — the CRUD flags are NO LONGER hardcoded to 'Y'. push-to-neo's
       // stepPopulateSpec hands populateWindowSpec a per-tab resolver built from
       // the SAME contract value read here (see buildEntityMethodFlagsResolver),
@@ -323,11 +328,20 @@ export function computeWindowDelta(args) {
     entityIdByNK,
     prevFieldByNatural,
     fieldUpserts,
+    closedEntityIds,
   });
 
   // ---- Mirror live-push behaviour: never create NEW records with ISINCLUDED=N
   // stepExcludeNonContractFields only updates EXISTING rows; it never inserts
   // a fresh ETGO_SF_FIELD record for a field that was never in the DB.
+  //
+  // ETP-4793 — this also prunes the field rows of a NEWLY closed entity, while
+  // the live `populateWindowSpec` does insert them with ISINCLUDED='N'. That gap
+  // is pre-existing and unchanged in kind (it already applied to every
+  // non-contract field of a brand-new window); on the reference instance all 90
+  // closed entities already have their rows in the prev snapshot, so they are
+  // kept and flipped. Widening the rule was deliberately left out of this change
+  // rather than folded in unmeasured.
   const prunedFieldUpserts = fieldUpserts.filter(
     f => prevFieldByNatural.has(f._naturalKey) || f.ISINCLUDED === 'Y',
   );
@@ -563,6 +577,7 @@ function applyContractVisibilityToFields({
   entityIdByNK,
   prevFieldByNatural,
   fieldUpserts,
+  closedEntityIds = new Set(),
 }) {
   const contractFields = extractFieldsFromContract(contract.backendContract);
   const fieldDefaultExprs = buildFieldDefaultExprMap(decisions);
@@ -595,6 +610,15 @@ function applyContractVisibilityToFields({
   }
 
   for (const row of fieldUpserts) {
+    // ETP-4793 — a field of an entity the contract does not declare is closed
+    // outright, before the flat-column-set rule below gets a say. That rule is
+    // entity-blind on purpose (it mirrors `stepExcludeNonContractFields`
+    // bug-for-bug), which is exactly why an excluded entity's `Name` column used
+    // to survive: some OTHER entity's contract has a column called `Name`.
+    if (closedEntityIds.has(row.ETGO_SF_ENTITY_ID)) {
+      row.ISINCLUDED = 'N';
+      continue;
+    }
     const colHit = contractFieldByEntityAndColumn.get(
       `${row.ETGO_SF_ENTITY_ID}/${String(row.AD_COLUMN_ID)}`,
     );