From 3a2b73ab4593220d217d0f8567952efdbd32f9eb Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Thu, 6 Aug 2026 12:00:35 -0300 Subject: [PATCH 1/6] Feature ETP-4793: Publish preview packages without requiring an open PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preview job was gated on the branch having an open PR, so a push to a PR-less feature branch ended green after the lookup with no tests, no publish and no cleanup — see run 31113050482 on feature/ETP-4793. The gate was in the wrong place. What consumes a preview is 'make bump-core-version VERSION=...', which needs the published version and nothing else; the PR was never a technical requirement. And a branch is often worth previewing before it is ready for review, which is exactly when the published-package path most needs exercising. find-pr no longer decides anything: it looks the PR up, and its only consumer is the sticky-comment step, now gated at step level. Without a PR the publish happens and the version is read off the 'preview-package' commit status, which is the surface that always exists. Nothing else depended on the PR — preview-version.mjs and cleanup-preview-packages.mjs both derive everything from the branch id. --- .github/workflows/publish-preview.yml | 28 +++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/.github/workflows/publish-preview.yml b/.github/workflows/publish-preview.yml index ad3a049d3..e2828d73e 100644 --- a/.github/workflows/publish-preview.yml +++ b/.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: -preview.... (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: " 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 }} From 0b712f8432966494ee6eb714301babc1458a35d0 Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Thu, 6 Aug 2026 11:47:10 -0300 Subject: [PATCH 2/6] Feature ETP-4793: Persist curated field visibility on push to NEO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IMP-11. ETGO_SF_FIELD.VISIBILITY existed in the DDL, the model and the generated entity class, and McpSchemaFieldBuilder already serialized it — but no writer ever populated it, so all 6340 rows were NULL and neo_schema never emitted the key. Two independent gaps closed: - neo-writer.js upsertField: the column was absent from the INSERT list and from the partial UPDATE. Added, routed through a new normalizeVisibility() that rejects values outside the curated vocabulary instead of storing garbage the reader would then serve as truth. NULL stays legal and means 'not classified' — populateSpec creates a row per AD column before any contract is applied. - push-to-neo.js buildFieldUpdateParams: had f.visibility in hand and discarded it. Now forwarded alongside the mapVisibility pair, not instead of it. isIncluded/isReadOnly are unchanged, so NEO runtime behaviour is untouched. reportDryRunPlan mirrors it so --dry-run stops lying. mapVisibility collapses four curated values into two booleans: system and readOnly both map to Y/Y, discarded and unknown both to N/N. That collapse is what the runtime wants and is also why the curated value must travel separately — the agent-facing hint tells agents to skip system fields and display readOnly ones, which Y/Y cannot express. Rewrote the two agent_prompt INSERT assertions that asserted 'the last param': positional assertions silently move onto the wrong column when a column is appended. They now resolve the index from the SQL column list. --- cli/src/neo-writer.js | 48 ++++++++- cli/src/push-to-neo.js | 13 ++- cli/test/neo-writer-upsert-field.test.js | 124 +++++++++++++++++++++-- cli/test/push-to-neo-helpers.test.js | 47 +++++++++ 4 files changed, 219 insertions(+), 13 deletions(-) diff --git a/cli/src/neo-writer.js b/cli/src/neo-writer.js index 5d70394e9..ca1276444 100644 --- a/cli/src/neo-writer.js +++ b/cli/src/neo-writer.js @@ -26,6 +26,39 @@ const SYSTEM_COLUMNS = [ 'updatedby', ]; +/** + * The four field visibility values of the curated schema, as stored in + * ETGO_SF_FIELD.VISIBILITY (VARCHAR(20)) and read back by the MCP layer + * (McpSchemaFieldBuilder) to emit `visibility` / `userRequired` on neo_schema. + * + * Kept as data rather than validated inline so the set has exactly one + * definition on the writer side. NULL is also legal and means "not classified": + * populateSpec creates a row per AD column before any contract is applied, and + * columns the contract never mentions legitimately stay NULL. + */ +export const FIELD_VISIBILITIES = ['editable', 'readOnly', 'system', 'discarded']; + +/** + * Validate a visibility value on its way to the DB. + * + * Fails loudly on an unknown value instead of storing it: the MCP layer serves + * this column to agents as authoritative metadata, so a typo silently persisted + * here becomes a wrong instruction there. + * + * @param {string|null|undefined} visibility + * @returns {string|null} the value, or null when unclassified + */ +export function normalizeVisibility(visibility) { + if (visibility == null || visibility === '') return null; + if (!FIELD_VISIBILITIES.includes(visibility)) { + throw new Error( + `upsertField: invalid visibility "${visibility}" ` + + `(expected one of ${FIELD_VISIBILITIES.join(', ')}, or null)`, + ); + } + return visibility; +} + /** * Generate an Etendo-compatible UUID (32-char uppercase hex, no dashes). */ @@ -237,6 +270,10 @@ export async function upsertEntity(client, params) { * @param {string} [params.fieldId] - If provided, UPDATE instead of INSERT * @param {string} [params.isIncluded='Y'] * @param {string} [params.isReadOnly='N'] + * @param {string} [params.visibility] - Curated visibility (see FIELD_VISIBILITIES). + * Orthogonal to isIncluded/isReadOnly, which drive NEO runtime behaviour: this + * column is agent-facing metadata only, and preserves the distinction those two + * booleans collapse (`system` and `readOnly` share the same Y/Y pair). * @param {string} [params.defaultValue] * @param {string} [params.agentPrompt] - Per-field agent guidance for neo_schema * @param {string} [params.javaQualifier] @@ -294,6 +331,10 @@ export async function upsertField(client, params) { setClauses.push(`isbusinesscritical = $${paramIndex++}`); values.push(params.isBusinessCritical ?? 'N'); } + if ('visibility' in params) { + setClauses.push(`visibility = $${paramIndex++}`); + values.push(normalizeVisibility(params.visibility)); + } setClauses.push(`updated = $${paramIndex++}`); values.push(auditVals.updated); @@ -313,6 +354,7 @@ export async function upsertField(client, params) { const javaQualifier = params.javaQualifier ?? null; const seqNo = params.seqNo ?? null; const agentPrompt = params.agentPrompt ?? null; + const visibility = normalizeVisibility(params.visibility); const fieldId = generateId(); await client.query( @@ -320,13 +362,13 @@ export async function upsertField(client, params) { (etgo_sf_field_id, etgo_sf_entity_id, ad_column_id, ad_module_id, isincluded, isreadonly, isbusinesscritical, defaultvalue, java_qualifier, seqno, ad_client_id, ad_org_id, isactive, created, createdby, updated, updatedby, - agent_prompt) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18)`, + agent_prompt, visibility) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19)`, [fieldId, entityId, columnId, moduleId, isIncluded, isReadOnly, isBusinessCritical, defaultValue, javaQualifier, seqNo, auditVals.ad_client_id, auditVals.ad_org_id, auditVals.isactive, auditVals.created, auditVals.createdby, auditVals.updated, auditVals.updatedby, - agentPrompt], + agentPrompt, visibility], ); return { fieldId, created: true }; } diff --git a/cli/src/push-to-neo.js b/cli/src/push-to-neo.js index 4ae29b509..bd00a72f1 100755 --- a/cli/src/push-to-neo.js +++ b/cli/src/push-to-neo.js @@ -439,6 +439,11 @@ export function buildFieldUpdateParams(f, ctx, fieldId, entityId) { moduleId: ctx.moduleId, isIncluded: vis.isIncluded, isReadOnly: vis.isReadOnly, + // Stored alongside — not instead of — the two booleans above. mapVisibility + // collapses four curated values into two flags, which is what NEO's runtime + // needs but loses the distinction agents are told to act on (`system` and + // `readOnly` both map to Y/Y). neo_schema reads this column verbatim. + visibility: f.visibility ?? null, isBusinessCritical: f.businessCritical ? 'Y' : 'N', audit: ctx.auditOpts, }; @@ -468,7 +473,13 @@ function reportDryRunPlan({ allFields, specName, windowId, windowDisplayName, wi return { action: 'upsertField', entityName: f.entityName, - params: { entityId: '(from populate)', column: f.column, isIncluded: vis.isIncluded, isReadOnly: vis.isReadOnly }, + params: { + entityId: '(from populate)', + column: f.column, + isIncluded: vis.isIncluded, + isReadOnly: vis.isReadOnly, + visibility: f.visibility ?? null, + }, }; }), }; diff --git a/cli/test/neo-writer-upsert-field.test.js b/cli/test/neo-writer-upsert-field.test.js index 32c724ac9..1aef02f02 100644 --- a/cli/test/neo-writer-upsert-field.test.js +++ b/cli/test/neo-writer-upsert-field.test.js @@ -1,13 +1,14 @@ /** - * Tests for upsertField in neo-writer.js, focused on the agentPrompt column - * and the partial-update contract (only explicitly provided columns are SET). + * Tests for upsertField in neo-writer.js, focused on the agentPrompt and + * visibility columns and the partial-update contract (only explicitly provided + * columns are SET). * * Uses a lightweight mock pg client that records the SQL and params of the * UPDATE/INSERT it receives, so no real database is needed. */ import { describe, it } from 'node:test'; import { strict as assert } from 'node:assert'; -import { upsertField } from '../src/neo-writer.js'; +import { upsertField, normalizeVisibility, FIELD_VISIBILITIES } from '../src/neo-writer.js'; function createMockClient() { const updates = []; @@ -27,6 +28,23 @@ function createMockClient() { }; } +/** + * Resolve the value bound to a named column of an INSERT, by reading the + * column list out of the SQL instead of assuming a position. Positional + * assertions ("the last param") silently pass onto the wrong column the next + * time a column is appended — which is exactly what happened when `visibility` + * was added after `agent_prompt`. + */ +function insertValue(insert, column) { + const columns = insert.sql + .slice(insert.sql.indexOf('(') + 1, insert.sql.indexOf(')')) + .split(',') + .map(c => c.trim()); + const index = columns.indexOf(column); + assert.notEqual(index, -1, `column ${column} is present in the INSERT`); + return insert.params[index]; +} + describe('upsertField (agentPrompt)', () => { it('SETs agent_prompt on UPDATE when provided', async () => { const client = createMockClient(); @@ -61,7 +79,7 @@ describe('upsertField (agentPrompt)', () => { assert.doesNotMatch(sql, /agent_prompt/); }); - it('persists agent_prompt as the last INSERT param', async () => { + it('persists agent_prompt on INSERT', async () => { const client = createMockClient(); await upsertField(client, { @@ -71,9 +89,7 @@ describe('upsertField (agentPrompt)', () => { }); assert.equal(client.inserts.length, 1); - const { sql, params } = client.inserts[0]; - assert.match(sql, /agent_prompt/); - assert.equal(params[params.length - 1], 'Hint for a new field.'); + assert.equal(insertValue(client.inserts[0], 'agent_prompt'), 'Hint for a new field.'); }); it('defaults agent_prompt to null on INSERT when omitted', async () => { @@ -81,7 +97,97 @@ describe('upsertField (agentPrompt)', () => { await upsertField(client, { entityId: 'ENT1', moduleId: 'MOD1' }); - const { params } = client.inserts[0]; - assert.equal(params[params.length - 1], null); + assert.equal(insertValue(client.inserts[0], 'agent_prompt'), null); + }); +}); + +describe('upsertField (visibility)', () => { + it('persists visibility on INSERT', async () => { + const client = createMockClient(); + + await upsertField(client, { entityId: 'ENT1', moduleId: 'MOD1', visibility: 'system' }); + + assert.equal(insertValue(client.inserts[0], 'visibility'), 'system'); + }); + + it('defaults visibility to null on INSERT when omitted', async () => { + const client = createMockClient(); + + await upsertField(client, { entityId: 'ENT1', moduleId: 'MOD1' }); + + assert.equal(insertValue(client.inserts[0], 'visibility'), null); + }); + + it('SETs visibility on UPDATE when provided', async () => { + const client = createMockClient(); + + await upsertField(client, { + entityId: 'ENT1', + moduleId: 'MOD1', + fieldId: 'FLD1', + visibility: 'readOnly', + }); + + const { sql, params } = client.updates[0]; + assert.match(sql, /visibility = \$\d+/); + assert.ok(params.includes('readOnly'), 'visibility value is bound in the UPDATE params'); + }); + + it('omits visibility from UPDATE when not provided (partial-update contract)', async () => { + const client = createMockClient(); + + await upsertField(client, { + entityId: 'ENT1', + moduleId: 'MOD1', + fieldId: 'FLD1', + isReadOnly: 'Y', + }); + + assert.doesNotMatch(client.updates[0].sql, /visibility/); + }); + + it('stores an explicit null when visibility is passed as null', async () => { + const client = createMockClient(); + + await upsertField(client, { + entityId: 'ENT1', + moduleId: 'MOD1', + fieldId: 'FLD1', + visibility: null, + }); + + const { sql, params } = client.updates[0]; + assert.match(sql, /visibility = \$\d+/); + assert.ok(params.includes(null), 'null is bound, clearing a stale classification'); + }); + + it('rejects a value outside the curated vocabulary', async () => { + const client = createMockClient(); + + await assert.rejects( + () => upsertField(client, { entityId: 'ENT1', moduleId: 'MOD1', visibility: 'hidden' }), + /invalid visibility "hidden"/, + ); + assert.equal(client.inserts.length, 0, 'nothing is written when validation fails'); + }); + + it('accepts every curated visibility value', async () => { + for (const visibility of FIELD_VISIBILITIES) { + const client = createMockClient(); + await upsertField(client, { entityId: 'ENT1', moduleId: 'MOD1', visibility }); + assert.equal(insertValue(client.inserts[0], 'visibility'), visibility); + } + }); +}); + +describe('normalizeVisibility', () => { + it('treats null and empty string as unclassified', () => { + assert.equal(normalizeVisibility(null), null); + assert.equal(normalizeVisibility(undefined), null); + assert.equal(normalizeVisibility(''), null); + }); + + it('is case-sensitive — the vocabulary is camelCase', () => { + assert.throws(() => normalizeVisibility('readonly'), /invalid visibility/); }); }); diff --git a/cli/test/push-to-neo-helpers.test.js b/cli/test/push-to-neo-helpers.test.js index bfc88772d..07197a397 100644 --- a/cli/test/push-to-neo-helpers.test.js +++ b/cli/test/push-to-neo-helpers.test.js @@ -11,8 +11,55 @@ import { formatDuplicateFieldsError, loadConfig, buildDesiredEntitiesMap, + buildFieldUpdateParams, } from '../src/push-to-neo.js'; +function updateParams(visibility) { + return buildFieldUpdateParams( + { entityName: 'header', fieldName: 'docStatus', column: 'DocStatus', visibility }, + { moduleId: 'MOD1', fieldDefaultExprs: {}, auditOpts: {} }, + 'FLD1', + 'ENT1', + ); +} + +describe('buildFieldUpdateParams (visibility passthrough)', () => { + it('forwards the curated visibility verbatim', () => { + assert.equal(updateParams('system').visibility, 'system'); + }); + + it('distinguishes system from readOnly even though both map to Y/Y', () => { + const system = updateParams('system'); + const readOnly = updateParams('readOnly'); + + // The booleans are deliberately identical — that collapse is what NEO's + // runtime wants, and it is also why the curated value must travel + // separately: neo_schema tells agents to skip system fields and display + // readOnly ones, a distinction Y/Y cannot express. + assert.equal(system.isIncluded, readOnly.isIncluded); + assert.equal(system.isReadOnly, readOnly.isReadOnly); + assert.notEqual(system.visibility, readOnly.visibility); + }); + + it('distinguishes discarded from an unknown value even though both map to N/N', () => { + const discarded = updateParams('discarded'); + const unknown = updateParams(undefined); + + assert.equal(discarded.isIncluded, unknown.isIncluded); + assert.equal(discarded.visibility, 'discarded'); + assert.equal(unknown.visibility, null, 'unclassified stays null, never a made-up value'); + }); + + it('keeps the mapVisibility pair untouched for every curated value', () => { + for (const visibility of ['editable', 'readOnly', 'system', 'discarded']) { + const params = updateParams(visibility); + const expected = mapVisibility(visibility); + assert.equal(params.isIncluded, expected.isIncluded); + assert.equal(params.isReadOnly, expected.isReadOnly); + } + }); +}); + describe('toSpecName', () => { it('converts simple display name to kebab-case', () => { assert.equal(toSpecName('Sales Order'), 'sales-order'); From 168498d076aeb6580bb88dfc612765f87882957a Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Wed, 12 Aug 2026 19:54:23 -0300 Subject: [PATCH 3/6] Feature ETP-4793: Reflect entity exclude:true as ISINCLUDED='N' in NEO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `exclude: true` only ever removed the entity from contract.json. Both write paths derive the ETGO_SF_ENTITY rows from ad_tab, never from the contract, so an excluded entity still got a row with ISINCLUDED='Y' plus one field row per AD column — 90 entities and 386 fields served on the reference instance, and served with MORE verbs than their curated siblings, because a tab with no contract entity falls through to the window-level method default. Adds one shared predicate, isEntityExcludedFromContract(), in the file both write paths already share, and threads it through: - push-to-neo: the method-flags resolver now also answers isIncluded, and the dry-run plan reports the entities it will close. - neo-writer: closes the entity and, with it, every field row underneath — redundant for behaviour (every reader filters the entity first) but field rows claiming 'Y' are what kept the gap invisible. - neo-delta: same flip on the predicted XML rows, applied BEFORE the entity-blind flat-column rule that let an excluded entity's column survive whenever a sibling contract entity had a column of the same name. The six method flags stay at the window default rather than being zeroed: they are unreachable on a closed entity, and an all-N set would break the GET/GETBYID invariant entity-methods.js enforces. VISIBILITY is left NULL — the XML delta does not model that column, and NULL beside 'N'/'N' is already the pair mapVisibility('discarded') produces. No schema change, no new AD records: ISINCLUDED already existed on both tables and 24 call sites across the REST and MCP surfaces already filter on it. --- cli/src/lib/entity-methods.js | 50 ++++++- cli/src/lib/neo-delta.js | 37 ++++- cli/src/neo-writer.js | 22 ++- cli/src/push-to-neo.js | 66 +++++++-- cli/test/entity-methods-pipeline.test.js | 178 +++++++++++++++++++++-- cli/test/neo-writer-populate.test.js | 119 +++++++++++++++ docs/decisions-reference.md | 30 +++- 7 files changed, 475 insertions(+), 27 deletions(-) diff --git a/cli/src/lib/entity-methods.js b/cli/src/lib/entity-methods.js index 1230eea63..9136925f3 100644 --- a/cli/src/lib/entity-methods.js +++ b/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..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..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..delete === false` (set by `window.hideDelete` / diff --git a/cli/src/lib/neo-delta.js b/cli/src/lib/neo-delta.js index f4ecc1fc5..ce59a9e33 100644 --- a/cli/src/lib/neo-delta.js +++ b/cli/src/lib/neo-delta.js @@ -39,7 +39,11 @@ import { newEtendoId } from './etendo-uuid.js'; import { indexByNaturalKey } from './etgo-xml-parser.js'; -import { methodsToXmlFlags, resolveContractEntityMethods } from './entity-methods.js'; +import { + isEntityExcludedFromContract, + methodsToXmlFlags, + resolveContractEntityMethods, +} from './entity-methods.js'; /** * Local copy of mapVisibility() from push-to-neo.js. Inlined to keep this @@ -249,6 +253,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 +281,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 +295,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 +337,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 +586,7 @@ function applyContractVisibilityToFields({ entityIdByNK, prevFieldByNatural, fieldUpserts, + closedEntityIds = new Set(), }) { const contractFields = extractFieldsFromContract(contract.backendContract); const fieldDefaultExprs = buildFieldDefaultExprMap(decisions); @@ -595,6 +619,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)}`, ); diff --git a/cli/src/neo-writer.js b/cli/src/neo-writer.js index f6385eda7..fbb91e487 100644 --- a/cli/src/neo-writer.js +++ b/cli/src/neo-writer.js @@ -540,12 +540,18 @@ async function populateWindowSpec(client, { specId, windowId, moduleId, excludeS // flag to 'N' when not supplied, so an entity must ALWAYS receive an explicit // set: the old `includeAllMethods: false` path left entities with no read // access, which is never a legitimate outcome. + // + // ETP-4793 — the resolver may also return `isIncluded: 'N'` for a tab the + // contract does not declare (`exclude: true`). The default resolver keeps + // `upsertEntity`'s own `'Y'` default, so callers that pass no resolver are + // unaffected. const resolveMethodFlags = (tab) => ( methodFlagsFor ? methodFlagsFor(tab) : { ...ALL_METHOD_FLAGS } ); let entityCount = 0; let fieldCount = 0; + let closedEntityCount = 0; const entities = []; const changes = { entities: { created: 0, updated: 0, deleted: 0 }, @@ -561,6 +567,17 @@ async function populateWindowSpec(client, { specId, windowId, moduleId, excludeS || existingEntityByName.get(tab.name) || null; + const entityFlags = resolveMethodFlags(tab); + // ETP-4793 — an entity the contract does not declare is closed, and so are + // its fields. Closing the entity alone would suffice for behaviour (every + // reader filters ETGO_SF_ENTITY.ISINCLUDED before it ever reaches a field), + // but leaving 15 field rows per closed entity claiming ISINCLUDED='Y' makes + // the data lie to anyone counting the agent surface — which is how the gap + // went unnoticed. `visibility` is deliberately NOT written here: the offline + // XML delta does not model that column at all (IMP-26 §4.2), and NULL next + // to 'N'/'N' is already the coherent pair under `mapVisibility`. + const entityIsClosed = entityFlags.isIncluded === 'N'; + if (entityIsClosed) closedEntityCount++; const { entityId, created: entityCreated } = await upsertEntity(client, { specId, tabId: tab.ad_tab_id, @@ -568,7 +585,7 @@ async function populateWindowSpec(client, { specId, windowId, moduleId, excludeS name: tab.name, seqNo: entitySeqNo, entityId: existingEntityId, - ...resolveMethodFlags(tab), + ...entityFlags, audit, }); entityCount++; @@ -618,6 +635,7 @@ async function populateWindowSpec(client, { specId, windowId, moduleId, excludeS moduleId, fieldId: existingFieldId, seqNo: fieldSeqCounter * 10, + ...(entityIsClosed ? { isIncluded: 'N' } : {}), audit, }); fieldCount++; @@ -633,7 +651,7 @@ async function populateWindowSpec(client, { specId, windowId, moduleId, excludeS // Delete stale entities (exist in DB but tab no longer in AD) await deleteStaleEntities(existingEntityByTab, visitedEntityIds, client, changes); - return { entityCount, fieldCount, entities, changes }; + return { entityCount, fieldCount, closedEntityCount, entities, changes }; } async function deleteDuplicateEntities(existingEntityResult, entityId, client, changes) { diff --git a/cli/src/push-to-neo.js b/cli/src/push-to-neo.js index 53c9f1bdc..17d67fe72 100755 --- a/cli/src/push-to-neo.js +++ b/cli/src/push-to-neo.js @@ -25,7 +25,12 @@ import { computeWindowDelta, serializeDelta } from './lib/neo-delta.js'; import { resolveAgentPromptRefs } from './lib/agent-prompt-ref.js'; import { loadEtgoXmlSnapshot } from './lib/etgo-xml-parser.js'; import { GO_MODULE_ID } from './lib/constants.js'; -import { methodsToWriterFlags, NEO_HTTP_METHODS, resolveContractEntityMethods } from './lib/entity-methods.js'; +import { + isEntityExcludedFromContract, + methodsToWriterFlags, + NEO_HTTP_METHODS, + resolveContractEntityMethods, +} from './lib/entity-methods.js'; import { isMainModule } from './utils.js'; const __filename = fileURLToPath(import.meta.url); @@ -286,7 +291,9 @@ export async function pushToNeo(windowName, options = {}) { const allFields = extractFieldsFromContract(contract.backendContract); if (options.dryRun === true) { - return reportDryRunPlan({ allFields, specName, windowId, windowDisplayName, windowName, contract }); + return reportDryRunPlan({ + allFields, specName, windowId, windowDisplayName, windowName, contract, schemaRawData, + }); } return executePushTransaction({ @@ -482,11 +489,35 @@ function summarizeRestrictedEntityMethods(contract) { return restricted; } -function reportDryRunPlan({ allFields, specName, windowId, windowDisplayName, windowName, contract }) { +/** + * ETP-4793 — the entities the push would close (`ISINCLUDED = 'N'`) because the + * contract does not declare them. Read off `schemaRawData`, not the contract, + * precisely because these entities are the ones the contract is missing. + * + * @param {object} contract - parsed contract.json + * @param {object} schemaRawData - parsed schema-raw.json + * @returns {string[]} curated entity names, sorted + */ +function summarizeExcludedEntities(contract, schemaRawData) { + const names = (schemaRawData?.entities ?? []) + .map((ent) => ent.name) + .filter((name) => isEntityExcludedFromContract(contract, name)); + return [...new Set(names)].sort(); +} + +function reportDryRunPlan({ + allFields, specName, windowId, windowDisplayName, windowName, contract, schemaRawData, +}) { const entityMethods = summarizeRestrictedEntityMethods(contract); + const excludedEntities = summarizeExcludedEntities(contract, schemaRawData); const plan = { spec: { action: 'upsertSpec', params: { windowId, name: specName, specType: 'W' } }, - populate: { action: 'populateSpec', params: { specId: '(from step 1)' }, entityMethods }, + populate: { + action: 'populateSpec', + params: { specId: '(from step 1)' }, + entityMethods, + excludedEntities, + }, fields: allFields.map(f => { const vis = mapVisibility(f.visibility); return { @@ -520,6 +551,12 @@ function reportDryRunPlan({ allFields, specName, windowId, windowDisplayName, wi console.log(` ${name}: ${entityMethods[name].join(', ')}`); } } + if (excludedEntities.length === 0) { + console.log(` Entities closed (exclude: true): none`); + } else { + console.log(` Entities closed (exclude: true) — ISINCLUDED='N' on entity + fields:`); + console.log(` ${excludedEntities.join(', ')}`); + } console.log(`\n Step 3: ${plan.fields.length} field updates via upsertField`); const included = plan.fields.filter(f => f.params.isIncluded === 'Y'); @@ -537,6 +574,7 @@ function reportDryRunPlan({ allFields, specName, windowId, windowDisplayName, wi included: included.length, excluded: excluded.length, readOnly: readOnly.length, + excludedEntities, }, }; } @@ -610,12 +648,19 @@ async function stepUpsertSpec(client, ctx) { * pure function of (contract, NAME), and NAME is already asserted equal between * the two paths by the XML regeneration check. * - * A tab with no contract entity (excluded entity, extra AD tab) falls through to - * the window-level default inside `resolveContractEntityMethods` — i.e. still - * read-only for a `window.readOnly` window, all methods otherwise. + * ETP-4793 — a tab with no contract entity (`exclude: true` in `decisions.json`, + * or an extra AD tab) now also gets `isIncluded: 'N'`, which is what actually + * closes it on both the REST and MCP surfaces; see + * `isEntityExcludedFromContract()` for why that column and not a new one. Its + * method flags still fall through to the window-level default, deliberately — + * they are unreachable once the entity is not included, and zeroing GET would + * break the invariant `entity-methods.js` enforces. + * + * `isIncluded` is `'Y'` for every entity the contract does declare, i.e. the + * value `upsertEntity` already defaults to, so nothing changes for them. * * @param {object} ctx - push context carrying `schemaRawData` + `contract` - * @returns {(tab: {name: string}) => object} writer method flags + * @returns {(tab: {name: string}) => object} writer entity flags, `isIncluded` included */ export function buildEntityMethodFlagsResolver({ schemaRawData, contract }) { const byTabName = new Map(); @@ -624,7 +669,10 @@ export function buildEntityMethodFlagsResolver({ schemaRawData, contract }) { } return (tab) => { const entityName = byTabName.get(tab?.name) ?? tab?.name; - return methodsToWriterFlags(resolveContractEntityMethods(contract, entityName)); + return { + isIncluded: isEntityExcludedFromContract(contract, entityName) ? 'N' : 'Y', + ...methodsToWriterFlags(resolveContractEntityMethods(contract, entityName)), + }; }; } diff --git a/cli/test/entity-methods-pipeline.test.js b/cli/test/entity-methods-pipeline.test.js index 2f4fded48..7d54f1a5a 100644 --- a/cli/test/entity-methods-pipeline.test.js +++ b/cli/test/entity-methods-pipeline.test.js @@ -20,10 +20,13 @@ import { generateContract } from '../src/generate-contract.js'; import { buildEntityMethodFlagsResolver } from '../src/push-to-neo.js'; import { computeWindowDelta } from '../src/lib/neo-delta.js'; -const ALL_Y = { isGet: 'Y', isGetbyid: 'Y', isPost: 'Y', isPut: 'Y', isPatch: 'Y', isDelete: 'Y' }; -const READ_ONLY = { isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'N', isPatch: 'N', isDelete: 'N' }; -const XML_ALL_Y = { ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'Y', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'Y' }; -const XML_READ_ONLY = { ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'N', ISPATCH: 'N', ISDELETE: 'N' }; +// ETP-4793 — `isIncluded` / `ISINCLUDED` joined this projection: an entity the +// contract DOES declare must stay included on both write paths, and the one it +// does not must be closed on both (see the `exclude: true` test below). +const ALL_Y = { isIncluded: 'Y', isGet: 'Y', isGetbyid: 'Y', isPost: 'Y', isPut: 'Y', isPatch: 'Y', isDelete: 'Y' }; +const READ_ONLY = { isIncluded: 'Y', isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'N', isPatch: 'N', isDelete: 'N' }; +const XML_ALL_Y = { ISINCLUDED: 'Y', ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'Y', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'Y' }; +const XML_READ_ONLY = { ISINCLUDED: 'Y', ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'N', ISPATCH: 'N', ISDELETE: 'N' }; // --------------------------------------------------------------------------- // Fixture: a two-tab window, mirroring a monitor/log window's shape. @@ -106,6 +109,7 @@ async function runChain(decisions) { const xmlFlags = {}; for (const row of delta.tables.ETGO_SF_ENTITY.upserts) { xmlFlags[row.NAME] = { + ISINCLUDED: row.ISINCLUDED, ISGET: row.ISGET, ISGETBYID: row.ISGETBYID, ISPOST: row.ISPOST, ISPUT: row.ISPUT, ISPATCH: row.ISPATCH, ISDELETE: row.ISDELETE, }; @@ -117,6 +121,7 @@ async function runChain(decisions) { /** Assert the live-DB flags and the predicted XML flags describe the same row. */ function assertPathsAgree(writerFlags, xmlFlags) { const pairs = [ + ['isIncluded', 'ISINCLUDED'], ['isGet', 'ISGET'], ['isGetbyid', 'ISGETBYID'], ['isPost', 'ISPOST'], ['isPut', 'ISPUT'], ['isPatch', 'ISPATCH'], ['isDelete', 'ISDELETE'], ]; @@ -199,10 +204,10 @@ describe('ETP-4254 — decisions → contract → both write paths', () => { assert.deepEqual(crud.log.methods, ['GET', 'GETBYID', 'PUT', 'PATCH']); assert.deepEqual(writerFlags.log, { - isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'Y', isPatch: 'Y', isDelete: 'N', + isIncluded: 'Y', isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'Y', isPatch: 'Y', isDelete: 'N', }); assert.deepEqual(xmlFlags.log, { - ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'N', + ISINCLUDED: 'Y', ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'N', }); assertPathsAgree(writerFlags, xmlFlags); }); @@ -240,10 +245,52 @@ describe('ETP-4254 — decisions → contract → both write paths', () => { } }); - it('an AD tab with no contract entity still follows the window default', async () => { + // ETP-4793 — an AD tab with no contract entity is now CLOSED (ISINCLUDED='N'), + // which is the column every NEO/MCP reader filters on before it resolves an + // entity. Its method flags deliberately keep following the window default: + // they are unreachable once the entity is not included, and an all-'N' set + // would break the GET/GETBYID invariant `entity-methods.js` enforces. + it('an AD tab with no contract entity is closed, methods still the window default', async () => { const { contract } = await runChain({ version: 'v2', window: { readOnly: true }, entities: {} }); const resolver = buildEntityMethodFlagsResolver({ schemaRawData: schemaRaw(), contract }); - assert.deepEqual(resolver({ ad_tab_id: 'T9', name: 'someExtraTab' }), READ_ONLY); + const flags = resolver({ ad_tab_id: 'T9', name: 'someExtraTab' }); + + assert.equal(flags.isIncluded, 'N', 'a tab absent from the contract must not be served'); + assert.deepEqual( + { ...flags, isIncluded: 'Y' }, READ_ONLY, + 'the method flags themselves are untouched — GET/GETBYID stay granted', + ); + }); + + it('exclude: true closes the entity on BOTH write paths', async () => { + const { contract, writerFlags, xmlFlags } = await runChain({ + version: 'v2', window: {}, entities: { logLine: { exclude: true } }, + }); + + assert.equal( + contract.backendContract.entities.logLine, undefined, + 'exclude: true keeps the entity out of the contract — the precondition being tested', + ); + assert.equal(writerFlags.logLine.isIncluded, 'N', 'live push must close it'); + assert.equal(xmlFlags.logLine.ISINCLUDED, 'N', 'XML delta must close the same row'); + assert.equal(writerFlags.log.isIncluded, 'Y', 'the sibling the contract DOES declare stays open'); + assert.equal(xmlFlags.log.ISINCLUDED, 'Y'); + assertPathsAgree(writerFlags, xmlFlags); + }); + + // A contract that declares NO entities at all (an older or half-generated + // artifact) must not be read as "everything is excluded" — that would close a + // whole window on one malformed file. + it('an empty contract closes nothing', async () => { + const resolver = buildEntityMethodFlagsResolver({ + schemaRawData: schemaRaw(), contract: { backendContract: { entities: {} } }, + }); + assert.equal(resolver({ ad_tab_id: 'T1', name: 'Conversion Rate Log' }).isIncluded, 'Y'); + assert.equal( + buildEntityMethodFlagsResolver({ schemaRawData: schemaRaw(), contract: {} })( + { ad_tab_id: 'T1', name: 'Conversion Rate Log' }, + ).isIncluded, 'Y', + ); }); // ETP-4745 — `hideDelete` previously only reached `apiPrediction.crud..delete` @@ -258,11 +305,11 @@ describe('ETP-4254 — decisions → contract → both write paths', () => { assert.equal(crud.log.delete, false); assert.equal(crud.logLine.delete, true); assert.deepEqual(writerFlags.log, { - isGet: 'Y', isGetbyid: 'Y', isPost: 'Y', isPut: 'Y', isPatch: 'Y', isDelete: 'N', + isIncluded: 'Y', isGet: 'Y', isGetbyid: 'Y', isPost: 'Y', isPut: 'Y', isPatch: 'Y', isDelete: 'N', }); assert.deepEqual(writerFlags.logLine, ALL_Y, 'sibling entity must keep DELETE'); assert.deepEqual(xmlFlags.log, { - ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'Y', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'N', + ISINCLUDED: 'Y', ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'Y', ISPUT: 'Y', ISPATCH: 'Y', ISDELETE: 'N', }); assert.deepEqual(xmlFlags.logLine, XML_ALL_Y); assertPathsAgree(writerFlags, xmlFlags); @@ -297,10 +344,10 @@ describe('ETP-4254 — decisions → contract → both write paths', () => { 'contract.methods still reflects the declared allowlist verbatim'); assert.equal(crud.log.delete, false); assert.deepEqual(writerFlags.log, { - isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'Y', isPatch: 'N', isDelete: 'N', + isIncluded: 'Y', isGet: 'Y', isGetbyid: 'Y', isPost: 'N', isPut: 'Y', isPatch: 'N', isDelete: 'N', }); assert.deepEqual(xmlFlags.log, { - ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'Y', ISPATCH: 'N', ISDELETE: 'N', + ISINCLUDED: 'Y', ISGET: 'Y', ISGETBYID: 'Y', ISPOST: 'N', ISPUT: 'Y', ISPATCH: 'N', ISDELETE: 'N', }); assertPathsAgree(writerFlags, xmlFlags); }); @@ -314,3 +361,110 @@ describe('ETP-4254 — decisions → contract → both write paths', () => { assertPathsAgree(writerFlags, xmlFlags); }); }); + +// --------------------------------------------------------------------------- +// ETP-4793 — the field-level half of `exclude: true`. +// +// Closing the entity row alone would be enough for behaviour (every reader +// filters ETGO_SF_ENTITY.ISINCLUDED before it resolves a field), but the field +// rows kept claiming ISINCLUDED='Y', which is how the gap stayed invisible: 386 +// such rows on the reference instance. The reason they survived is specific and +// worth pinning — `stepExcludeNonContractFields` (and the delta that mirrors it +// bug-for-bug) compares column NAMES against a FLAT set gathered across ALL +// contract entities, so an excluded entity's column survives whenever a sibling +// contract entity happens to have a column of the same name. +// +// The fixture below is built for exactly that: the excluded `logLine` tab gets a +// `DocumentNo` column, which the contract's `log` entity also has. +// --------------------------------------------------------------------------- + +const SPEC = 'conversion-rate-downloader-log'; +const SHARED_COL = { ad_column_id: 'C4', ad_table_id: 'TBL2', columnname: 'DocumentNo' }; + +/** + * schemaRaw() with an explicit `editable` visibility on every field, plus a + * `DocumentNo` column on the second entity so it collides with the first's. + * + * The visibility matters: `mapVisibility(undefined)` is `N`/`N`, so the base + * fixture — which never asserted on field rows — would show every field closed + * and prove nothing. + */ +function schemaRawWithSharedColumn() { + const raw = schemaRaw(); + for (const entity of raw.entities) { + for (const field of entity.fields) field.visibility = 'editable'; + } + raw.entities[1].fields.push({ + name: 'documentNo', columnName: 'DocumentNo', type: 'string', reference: 'String', + visibility: 'editable', + }); + return raw; +} + +/** + * A prev-XML snapshot holding the rows this window already has, so the delta's + * "never create NEW records with ISINCLUDED=N" pruning keeps them and the + * assertions can see the flip. That mirrors the live instance, where all 90 + * closed entities already exist in the exported XML. + */ +function prevSnapshotFor(columns) { + const entityIdByTab = { T1: 'E1', T2: 'E2' }; + const entityIdByTable = { TBL1: 'E1', TBL2: 'E2' }; + return { + spec: [{ ETGO_SF_SPEC_ID: 'S1', NAME: SPEC }], + entity: AD_TABS.map(tab => ({ + ETGO_SF_ENTITY_ID: entityIdByTab[tab.ad_tab_id], + ETGO_SF_SPEC_ID: 'S1', + AD_TAB_ID: tab.ad_tab_id, + })), + field: columns.map((col, i) => ({ + ETGO_SF_FIELD_ID: `F${i + 1}`, + ETGO_SF_ENTITY_ID: entityIdByTable[col.ad_table_id], + AD_COLUMN_ID: col.ad_column_id, + })), + }; +} + +/** Compute the XML delta for a window whose second entity is `exclude: true`. */ +async function deltaWithExcludedLogLine() { + const raw = schemaRawWithSharedColumn(); + const columns = [...AD_COLUMNS, SHARED_COL]; + const decisions = { version: 'v2', window: {}, entities: { logLine: { exclude: true } } }; + const { schema } = await resolveCurated(raw, { rules: [] }, decisions); + const contract = generateContract(schema, [], []); + const delta = computeWindowDelta({ + specName: SPEC, + windowId: 'W001', + moduleId: 'AABBCCDD11223344', + contract, + decisions, + adTabs: AD_TABS, + adColumns: columns, + prevSnapshot: prevSnapshotFor(columns), + schemaRawData: raw, + }); + const byColumn = {}; + for (const row of delta.tables.ETGO_SF_FIELD.upserts) byColumn[row.AD_COLUMN_ID] = row; + return { contract, delta, byColumn }; +} + +describe('ETP-4793 — fields of an entity excluded from the contract', () => { + it('are closed even when a declared entity shares the column name', async () => { + const { contract, byColumn } = await deltaWithExcludedLogLine(); + + assert.equal(contract.backendContract.entities.logLine, undefined); + assert.equal( + byColumn.C4?.ISINCLUDED, 'N', + "logLine.DocumentNo must be closed — the flat column set alone would keep it 'Y' " + + 'because log.DocumentNo IS in the contract', + ); + assert.equal(byColumn.C3?.ISINCLUDED, 'N', 'logLine.Rate is closed as well'); + }); + + it('leaves the fields of the declared entity untouched', async () => { + const { byColumn } = await deltaWithExcludedLogLine(); + + assert.equal(byColumn.C1?.ISINCLUDED, 'Y', 'log.DocumentNo is contract-declared'); + assert.equal(byColumn.C2?.ISINCLUDED, 'Y', 'log.Status is contract-declared'); + }); +}); diff --git a/cli/test/neo-writer-populate.test.js b/cli/test/neo-writer-populate.test.js index 10e27a927..012d0e114 100644 --- a/cli/test/neo-writer-populate.test.js +++ b/cli/test/neo-writer-populate.test.js @@ -749,3 +749,122 @@ describe('populateSpec (report)', () => { ); }); }); + +// --------------------------------------------------------------------------- +// ETP-4793 — `exclude: true` on the live write path +// --------------------------------------------------------------------------- + +/** + * `methodFlagsFor` now carries an `isIncluded` axis alongside the six HTTP verbs + * (see `buildEntityMethodFlagsResolver` in push-to-neo.js). When it answers 'N' + * for a tab, `populateWindowSpec` must close BOTH the ETGO_SF_ENTITY row and + * every ETGO_SF_FIELD row underneath it — closing the entity alone would be + * enough for behaviour, since every reader filters ISINCLUDED before it resolves + * a field, but field rows still claiming 'Y' misreport the agent surface. + */ +describe('populateSpec (window) — entities closed by exclude: true', () => { + const SPEC_ID = 'SPEC001'; + const MODULE_ID = 'MOD001'; + const WINDOW_ID = 'WIN001'; + const OPEN_TAB = 'TAB001'; + const CLOSED_TAB = 'TAB002'; + const OPEN_TABLE = 'TBL001'; + const CLOSED_TABLE = 'TBL002'; + + const ALL_VERBS_Y = { + isGet: 'Y', isGetbyid: 'Y', isPost: 'Y', isPut: 'Y', isPatch: 'Y', isDelete: 'Y', + }; + + function clientWithOneClosedTab() { + return createMockClient({ + specs: [[SPEC_ID, { spec_type: 'W', ad_window_id: WINDOW_ID, ad_process_id: null }]], + tabs: [ + { ad_tab_id: OPEN_TAB, name: 'Header', ad_table_id: OPEN_TABLE, seqno: 10, ad_window_id: WINDOW_ID }, + { ad_tab_id: CLOSED_TAB, name: 'Audit Log', ad_table_id: CLOSED_TABLE, seqno: 20, ad_window_id: WINDOW_ID }, + ], + columns: [ + { ad_column_id: 'COL001', columnname: 'DocumentNo', position: 10, ad_table_id: OPEN_TABLE }, + // Same column NAME on both tables — the entity-blind flat-column rule in + // push-to-neo would keep this one open, so the entity-level close has to + // be what shuts it. + { ad_column_id: 'COL002', columnname: 'DocumentNo', position: 10, ad_table_id: CLOSED_TABLE }, + { ad_column_id: 'COL003', columnname: 'Rate', position: 20, ad_table_id: CLOSED_TABLE }, + ], + }); + } + + /** The tab whose contract entity is missing answers isIncluded: 'N'. */ + const methodFlagsFor = (tab) => ({ + isIncluded: tab.ad_tab_id === CLOSED_TAB ? 'N' : 'Y', + ...ALL_VERBS_Y, + }); + + /** Entity INSERT param order: [id, specId, tabId, ?, name, isincluded, …]. */ + function entityInsertsByName(client) { + const byName = {}; + for (const q of client.queryLog) { + if (q.sql.includes('INSERT INTO etgo_sf_entity')) { + byName[q.params[4]] = { entityId: q.params[0], isIncluded: q.params[5] }; + } + } + return byName; + } + + /** Field INSERT param order: [id, entityId, columnId, moduleId, isincluded, …]. */ + function fieldInsertsByColumn(client) { + const byColumn = {}; + for (const q of client.queryLog) { + if (q.sql.includes('INSERT INTO etgo_sf_field')) { + byColumn[q.params[2]] = { entityId: q.params[1], isIncluded: q.params[4] }; + } + } + return byColumn; + } + + it("writes ISINCLUDED='N' on the closed entity and 'Y' on its sibling", async () => { + const client = clientWithOneClosedTab(); + + await populateSpec(client, { specId: SPEC_ID, moduleId: MODULE_ID, methodFlagsFor }); + + const entities = entityInsertsByName(client); + assert.equal(entities.Header.isIncluded, 'Y'); + assert.equal(entities['Audit Log'].isIncluded, 'N'); + }); + + it("closes every field of the closed entity, sharing a column name or not", async () => { + const client = clientWithOneClosedTab(); + + await populateSpec(client, { specId: SPEC_ID, moduleId: MODULE_ID, methodFlagsFor }); + + const fields = fieldInsertsByColumn(client); + assert.equal(fields.COL001.isIncluded, 'Y', 'Header.DocumentNo stays open'); + assert.equal( + fields.COL002.isIncluded, 'N', + 'Audit Log.DocumentNo is closed despite Header having a DocumentNo too', + ); + assert.equal(fields.COL003.isIncluded, 'N', 'Audit Log.Rate is closed'); + }); + + it('reports the closed entities in closedEntityCount', async () => { + const client = clientWithOneClosedTab(); + + const result = await populateSpec(client, { specId: SPEC_ID, moduleId: MODULE_ID, methodFlagsFor }); + + assert.equal(result.entityCount, 2, 'a closed entity is still written, not skipped'); + assert.equal(result.fieldCount, 3); + assert.equal(result.closedEntityCount, 1); + }); + + it('counts nothing closed when every tab is included', async () => { + const client = clientWithOneClosedTab(); + + const result = await populateSpec(client, { + specId: SPEC_ID, + moduleId: MODULE_ID, + methodFlagsFor: () => ({ isIncluded: 'Y', ...ALL_VERBS_Y }), + }); + + assert.equal(result.closedEntityCount, 0); + for (const f of Object.values(fieldInsertsByColumn(client))) assert.equal(f.isIncluded, 'Y'); + }); +}); diff --git a/docs/decisions-reference.md b/docs/decisions-reference.md index 39bc9e0c8..29906820f 100644 --- a/docs/decisions-reference.md +++ b/docs/decisions-reference.md @@ -526,7 +526,7 @@ Entity keys use **camelCase from tabName** (e.g., `"header"`, `"lines"`, `"basic | Property | Type | Default | Purpose | |----------|------|---------|---------| | `name` | string | Entity key | Override display name. | -| `exclude` | boolean | `false` | Omit entire entity from schema. | +| `exclude` | boolean | `false` | Omit entire entity from schema **and close it in NEO** (`ETGO_SF_ENTITY.ISINCLUDED = 'N'` on the entity plus every one of its `ETGO_SF_FIELD` rows). See below. | | `fields` | object | `{}` | Field-level decisions. | | `draftMode` | object | `null` | Draft/Processed workflow config. | | `javaQualifier` | string | `null` | CDI qualifier for custom NeoHandler. | @@ -1084,6 +1084,34 @@ Rule keys use **extended names** (including trigger column suffix for multi-trig } ``` +**What it does (ETP-4793).** The entity is dropped from the curated schema, and so +from `contract.json`. Both write paths then close it in NEO: + +| Row | Column | Value | +|---|---|---| +| `ETGO_SF_ENTITY` (the excluded entity) | `ISINCLUDED` | `N` | +| `ETGO_SF_FIELD` (every field of it) | `ISINCLUDED` | `N` | + +`ISINCLUDED = 'N'` on the entity is what actually removes it from the served +surface — every REST and MCP reader filters on it before resolving an entity, so +neither `GET /{entity}` nor an MCP `neo_list` can reach it. The field rows are +closed too: it is redundant for behaviour, but a closed entity whose 15 field rows +still claim `ISINCLUDED = 'Y'` misreports the size of the agent surface, which is +exactly how the gap went unnoticed for 90 entities / 386 fields. + +Two deliberate non-changes: + +- **The six HTTP method flags are left at the window default, not zeroed.** They + are unreachable once the entity is closed, and an all-`N` set would break the + "GET and GETBYID are always granted" invariant that `cli/src/lib/entity-methods.js` + enforces. Leaving them alone also keeps the XML diff to one column per entity. +- **`VISIBILITY` is not written on the closed field rows.** The offline XML delta + does not model that column, and NULL beside `ISINCLUDED='N'` / `ISREADONLY='N'` + is already the pair `mapVisibility('discarded')` produces. + +Excluding an entity changes `ETGO_SF_*`, so it needs a re-push and an export: +`make regen ONLY= PUSH_TO_NEO=1` then `./gradlew export.database`. + ### Custom NeoHandler for an entity ```json { From d2fddd4f65534969210957a154a651ae6d47da76 Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Wed, 12 Aug 2026 19:58:30 -0300 Subject: [PATCH 4/6] Feature ETP-4793: Give the excluded-entity sort an explicit comparator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sonar javascript:S2871 — Array.prototype.sort() with no compare function coerces elements to strings, so it only happens to be right for an array of names. Sorting entity names with localeCompare is what the rest of the CLI does (check-window-docs.js, check-version.js) and is behaviour-preserving here. --- cli/src/push-to-neo.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/push-to-neo.js b/cli/src/push-to-neo.js index 17d67fe72..3b6ce5678 100755 --- a/cli/src/push-to-neo.js +++ b/cli/src/push-to-neo.js @@ -502,7 +502,7 @@ function summarizeExcludedEntities(contract, schemaRawData) { const names = (schemaRawData?.entities ?? []) .map((ent) => ent.name) .filter((name) => isEntityExcludedFromContract(contract, name)); - return [...new Set(names)].sort(); + return [...new Set(names)].sort((left, right) => left.localeCompare(right)); } function reportDryRunPlan({ From 89e76d82297c474dbc6c74402296542d7d3b282e Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Thu, 13 Aug 2026 09:29:00 -0300 Subject: [PATCH 5/6] Feature ETP-4793: Add validator rule F23 for pushed field visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IMP-26 §5.3 asked for a recurrence guard on the ETGO_SF_FIELD invariant mapVisibility(visibility) == (isincluded, isreadonly). F23 reads the exported ETGO_SF_*.xml sourcedata (the only DB-free view of pushed state) and re-runs the projection over it. Two classes, scored differently: BLOCK when a curated VISIBILITY projects to a different flag pair than the one stored (writer bug or hand-edit), WARN when VISIBILITY was never written while the flags say included (neo_schema then reports no visibility, so an agent cannot tell readOnly from system). One BLOCKing rule would have gone red on 409 rows on day one. An absent VISIBILITY on a closed row is coherent, not debt: N/N is mapVisibility(null), which is the shape the exclude: true fix produces. Registered for window AND aggregate artifacts. Window-only reported 69 of the 409 incoherent rows in the live export; the other 340 sit in two aggregates. mapVisibility moves to lib/field-visibility.js. It existed twice (exported from push-to-neo, inlined in neo-delta to dodge a circular import) and F23 would have been a third copy — a validator that re-implements the projection cannot detect a drift in the projection. push-to-neo re-exports it as public API. Measured against the live DB and the XML independently, agreeing: 0 contradictions, 409 unwritten of 6,468 active rows, in 6 specs. --- cli/src/lib/field-visibility.js | 96 +++++++++ cli/src/lib/neo-delta.js | 27 +-- cli/src/push-to-neo.js | 20 +- cli/src/validate-pipeline.js | 200 +++++++++++++++++- cli/test/field-visibility.test.js | 97 +++++++++ .../ETGO_SF_ENTITY.xml | 10 + .../ETGO_SF_FIELD.xml | 13 ++ .../f23-sourcedata-aggregate/ETGO_SF_SPEC.xml | 9 + .../f23-sourcedata-blank/ETGO_SF_ENTITY.xml | 10 + .../f23-sourcedata-blank/ETGO_SF_FIELD.xml | 10 + .../f23-sourcedata-blank/ETGO_SF_SPEC.xml | 9 + .../f23-sourcedata-clean/ETGO_SF_ENTITY.xml | 10 + .../f23-sourcedata-clean/ETGO_SF_FIELD.xml | 57 +++++ .../f23-sourcedata-clean/ETGO_SF_SPEC.xml | 9 + .../ETGO_SF_ENTITY.xml | 10 + .../ETGO_SF_FIELD.xml | 24 +++ .../ETGO_SF_SPEC.xml | 9 + .../f23-sourcedata-empty/ETGO_SF_ENTITY.xml | 3 + .../f23-sourcedata-empty/ETGO_SF_FIELD.xml | 3 + .../f23-sourcedata-empty/ETGO_SF_SPEC.xml | 3 + .../ETGO_SF_ENTITY.xml | 10 + .../f23-sourcedata-inactive/ETGO_SF_FIELD.xml | 25 +++ .../f23-sourcedata-inactive/ETGO_SF_SPEC.xml | 9 + .../ETGO_SF_ENTITY.xml | 10 + .../ETGO_SF_FIELD.xml | 34 +++ .../f23-sourcedata-unwritten/ETGO_SF_SPEC.xml | 9 + cli/test/validate-pipeline.test.js | 103 +++++++++ docs/pipeline-validator-reference.md | 2 + 28 files changed, 789 insertions(+), 42 deletions(-) create mode 100644 cli/src/lib/field-visibility.js create mode 100644 cli/test/field-visibility.test.js create mode 100644 cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_SPEC.xml create mode 100644 cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_ENTITY.xml create mode 100644 cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_FIELD.xml create mode 100644 cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_SPEC.xml diff --git a/cli/src/lib/field-visibility.js b/cli/src/lib/field-visibility.js new file mode 100644 index 000000000..9ad79b7dc --- /dev/null +++ b/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, + }; +} diff --git a/cli/src/lib/neo-delta.js b/cli/src/lib/neo-delta.js index ce59a9e33..d420bc1e5 100644 --- a/cli/src/lib/neo-delta.js +++ b/cli/src/lib/neo-delta.js @@ -44,22 +44,11 @@ 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' }; - } -} +// 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; @@ -69,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) { diff --git a/cli/src/push-to-neo.js b/cli/src/push-to-neo.js index 3b6ce5678..a41e98639 100755 --- a/cli/src/push-to-neo.js +++ b/cli/src/push-to-neo.js @@ -24,6 +24,7 @@ import { import { computeWindowDelta, serializeDelta } from './lib/neo-delta.js'; import { resolveAgentPromptRefs } from './lib/agent-prompt-ref.js'; import { loadEtgoXmlSnapshot } from './lib/etgo-xml-parser.js'; +import { mapVisibility } from './lib/field-visibility.js'; import { GO_MODULE_ID } from './lib/constants.js'; import { isEntityExcludedFromContract, @@ -57,21 +58,12 @@ export function toSpecName(windowName) { /** * Map a field visibility value to NEO params. * Returns { isIncluded: "Y"|"N", isReadOnly: "Y"|"N" }. + * + * ETP-4793 — the implementation moved to `lib/field-visibility.js` so the + * offline delta path and validator rule F23 share it instead of re-declaring + * it. Re-exported here because this name is part of the module's public API. */ -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' }; - } -} +export { mapVisibility }; /** * Build the full webhook URL from a base Etendo URL and webhook name. diff --git a/cli/src/validate-pipeline.js b/cli/src/validate-pipeline.js index 2857cd07f..0ce5c3be1 100644 --- a/cli/src/validate-pipeline.js +++ b/cli/src/validate-pipeline.js @@ -30,6 +30,8 @@ import { resolveEntityHideDelete, resolveEntityMethods, } from './lib/entity-methods.js'; +import { parseEtgoXmlFile } from './lib/etgo-xml-parser.js'; +import { visibilityMatchesFlags } from './lib/field-visibility.js'; const execFileAsync = promisify(execFile); @@ -807,6 +809,170 @@ async function ruleF22(artifactDir, artifactName) { 'Remove window.customTabsAfterBottom, or remove tabOrder from the listed custom tab(s).'); } +// ─── F23 — ETGO_SF_FIELD visibility vs the flags it projects to ───────────── +// ETP-4793 / IMP-26 §5.3. `populateSpec` writes ISINCLUDED/ISREADONLY on every +// push but VISIBILITY only on some paths, so the same decision — stored twice +// on purpose, because neo_schema needs the curated word and the runtime needs +// the booleans — can drift apart silently. This rule reads the exported +// sourcedata and re-runs the projection over it. + +/** + * Resolve the com.etendoerp.go sourcedata directory holding the exported + * ETGO_SF_*.xml files. Mirrors `resolveDefaultPrevXmlDir` in push-to-neo.js. + * + * @param {string} root - schema_forge repo root + * @returns {string} + */ +function resolveGoSourcedataDir(root) { + if (process.env.SF_GO_SOURCEDATA_DIR) return process.env.SF_GO_SOURCEDATA_DIR; + const etendoRoot = process.env.ETENDO_ROOT || join(root, '..'); + return join(etendoRoot, 'modules', 'com.etendoerp.go', 'src-db', 'database', 'sourcedata'); +} + +// One 8 MB XML parse per process, not per artifact. Keyed by directory so a +// test pointing at a fixture never reads the cached production snapshot. +const f23SnapshotCache = new Map(); + +/** + * Parse the three sourcedata XMLs and index the field rows by spec name. + * + * Returns `null` — not an empty index — when the directory is absent, so the + * caller can tell "no .go checkout here" apart from "checkout present, spec has + * no rows". + * + * @param {string} sourcedataDir + * @returns {Promise>|null>} spec name → field rows + */ +async function loadF23FieldsBySpec(sourcedataDir) { + if (f23SnapshotCache.has(sourcedataDir)) return f23SnapshotCache.get(sourcedataDir); + const pending = (async () => { + const paths = { + spec: join(sourcedataDir, 'ETGO_SF_SPEC.xml'), + entity: join(sourcedataDir, 'ETGO_SF_ENTITY.xml'), + field: join(sourcedataDir, 'ETGO_SF_FIELD.xml'), + }; + const present = await Promise.all(Object.values(paths).map(fileExists)); + if (present.some(ok => !ok)) return null; + + const [specs, entities, fields] = await Promise.all([ + parseEtgoXmlFile(paths.spec, 'ETGO_SF_SPEC').then(r => r.rows), + parseEtgoXmlFile(paths.entity, 'ETGO_SF_ENTITY').then(r => r.rows), + parseEtgoXmlFile(paths.field, 'ETGO_SF_FIELD').then(r => r.rows), + ]); + + const specNameById = new Map(specs.map(s => [s.ETGO_SF_SPEC_ID, s.NAME])); + const entityById = new Map(entities.map(e => [e.ETGO_SF_ENTITY_ID, e])); + + const bySpec = new Map(); + for (const row of fields) { + if (row.ISACTIVE === 'N') continue; // retired config, not a live surface + const entity = entityById.get(row.ETGO_SF_ENTITY_ID); + const specName = entity && specNameById.get(entity.ETGO_SF_SPEC_ID); + if (!specName) continue; // orphan row — F23 is not the rule that owns that + if (!bySpec.has(specName)) bySpec.set(specName, []); + bySpec.get(specName).push({ row, entityName: entity.NAME }); + } + return bySpec; + })(); + f23SnapshotCache.set(sourcedataDir, pending); + return pending; +} + +/** + * Human-readable identity for one offending field row. + * + * `JAVA_QUALIFIER` is only written when the curated key differs from the AD + * column name, so `AD_COLUMN_ID` is the usual identity. Some pushed rows carry + * neither — ETP-4793 found 105 field rows in the live export with no + * AD_COLUMN_ID, JAVA_QUALIFIER or SEQNO at all, just the two flags. Those are + * degenerate rows pointing at no AD column; label them by primary key and say + * so, because "entity.undefined" tells the reader nothing. + */ +function f23FieldLabel({ row, entityName }) { + const entity = entityName || '?'; + const name = row.JAVA_QUALIFIER || row.AD_COLUMN_ID; + if (name) return `${entity}.${name}`; + return `${entity}.`; +} + +/** + * F23: a pushed `ETGO_SF_FIELD` row whose `VISIBILITY` does not project to its + * stored `ISINCLUDED`/`ISREADONLY` pair. + * + * Two failure modes, deliberately scored differently: + * - BLOCK `contradiction` — VISIBILITY holds a curated value that projects to + * a different pair than the one stored. Only a writer bug or a hand-edit + * gets you here, and it means the runtime and `neo_schema` disagree about + * the same field. + * - WARN `unwritten` — VISIBILITY is absent while the flags say the field is + * included. Harmless to the runtime, but `neo_schema` reports no visibility + * for that field, so an agent cannot tell `readOnly` from `system`. + * Pre-existing backfill debt; a warning so it is counted, not so it blocks. + * + * Inert (returns null) without a com.etendoerp.go checkout — the exported XML is + * the only DB-free view of pushed state, and this repo can be used alone. + * + * @param {string} artifactDir + * @param {string} artifactName - artifact dir name === spec name + * @param {string} root + * @param {string} [sourcedataDir] + * @returns {Promise} + */ +async function ruleF23(artifactDir, artifactName, root = ROOT, sourcedataDir) { + const dir = sourcedataDir ?? resolveGoSourcedataDir(root); + const bySpec = await loadF23FieldsBySpec(dir); + if (bySpec === null) return null; // no runtime-module checkout — nothing to read + const rows = bySpec.get(artifactName); + if (!rows || rows.length === 0) return null; // never pushed, or a different spec name + + const contradictions = []; + const unwritten = []; + for (const entry of rows) { + const verdict = visibilityMatchesFlags({ + visibility: entry.row.VISIBILITY, + isIncluded: entry.row.ISINCLUDED, + isReadOnly: entry.row.ISREADONLY, + }); + if (verdict.ok) continue; + (verdict.kind === 'contradiction' ? contradictions : unwritten).push({ entry, verdict }); + } + + if (contradictions.length > 0) { + const shown = contradictions.slice(0, 3).map(({ entry, verdict }) => ( + `${f23FieldLabel(entry)} (visibility='${entry.row.VISIBILITY}' projects to ` + + `ISINCLUDED=${verdict.expected.isIncluded}/ISREADONLY=${verdict.expected.isReadOnly}, ` + + `stored ${entry.row.ISINCLUDED}/${entry.row.ISREADONLY})` + )); + const more = contradictions.length > shown.length ? ` (+${contradictions.length - shown.length} more)` : ''; + return violation( + 'F23', artifactName, 'BLOCK', + `${contradictions.length} pushed ETGO_SF_FIELD row(s) store a VISIBILITY that contradicts their ` + + `ISINCLUDED/ISREADONLY flags: ${shown.join('; ')}${more}. The runtime enforces the flags while ` + + `neo_schema reports the visibility, so the two now describe the field differently.`, + `Re-push the window (make regen ONLY=${artifactName} PUSH_TO_NEO=1) and re-run ` + + `./gradlew export.database in Etendo root. If the drift survives a clean push, the writer is ` + + `at fault — fix cli/src/neo-writer.js populateSpec, not the XML.`, + { f23Contradictions: contradictions.length, f23Unwritten: unwritten.length }, + ); + } + + if (unwritten.length > 0) { + const shown = unwritten.slice(0, 3).map(({ entry }) => f23FieldLabel(entry)); + const more = unwritten.length > shown.length ? ` (+${unwritten.length - shown.length} more)` : ''; + return violation( + 'F23', artifactName, 'WARN', + `${unwritten.length} pushed ETGO_SF_FIELD row(s) are included (ISINCLUDED=Y) but carry no ` + + `VISIBILITY value: ${shown.join(', ')}${more}. neo_schema reports these fields with no ` + + `visibility, so an agent cannot distinguish readOnly from system.`, + `Re-push the window (make regen ONLY=${artifactName} PUSH_TO_NEO=1) then ` + + `./gradlew export.database — populateSpec writes VISIBILITY on the fields it revisits.`, + { f23Contradictions: 0, f23Unwritten: unwritten.length }, + ); + } + + return null; +} + /** * Collect the field names declared on the line (non-header / detail) entity of * the frontend contract. Real generated contracts populate @@ -1798,7 +1964,7 @@ async function runEnabledChecks(checks, skipSet) { return (await Promise.all(pendingChecks)).filter(Boolean); } -async function runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist = []) { +async function runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist = [], goSourcedataDir) { return runEnabledChecks([ { rule: 'F1', run: () => ruleF1(artifactDir, artifactName) }, { rule: 'F2', run: () => ruleF2(artifactDir, artifactName) }, @@ -1820,6 +1986,7 @@ async function runWindowChecks(artifactDir, artifactName, registryContent, root, { rule: 'F20', run: () => ruleF20(artifactDir, artifactName, root) }, { rule: 'F21', run: () => ruleF21(artifactDir, artifactName) }, { rule: 'F22', run: () => ruleF22(artifactDir, artifactName) }, + { rule: 'F23', run: () => ruleF23(artifactDir, artifactName, root, goSourcedataDir) }, ], skipSet); } @@ -1833,16 +2000,19 @@ function tagArtifactKind(results, artifactKind) { return results.map(result => ({ ...result, artifactKind })); } -async function runAggregateSectionChecks(artifactDir, artifactName, skipSet) { +async function runAggregateSectionChecks(artifactDir, artifactName, skipSet, root, goSourcedataDir) { const f9Results = await runSingleCheck('F9', () => ruleF9(artifactDir, artifactName), skipSet); const f4Results = await runSingleCheck('F4', () => ruleF4(artifactDir, artifactName), skipSet); - return [...f9Results, ...f4Results]; + const f23Results = await runSingleCheck( + 'F23', () => ruleF23(artifactDir, artifactName, root, goSourcedataDir), skipSet, + ); + return [...f9Results, ...f4Results, ...f23Results]; } -async function runChecksForArtifact({ kind, artifactDir, artifactName, registryContent, root, skipSet, strict, f19Allowlist }) { +async function runChecksForArtifact({ kind, artifactDir, artifactName, registryContent, root, skipSet, strict, f19Allowlist, goSourcedataDir }) { if (kind === 'window') { return tagArtifactKind( - await runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist), + await runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist, goSourcedataDir), 'window', ); } @@ -1853,14 +2023,19 @@ async function runChecksForArtifact({ kind, artifactDir, artifactName, registryC ); } if (kind === 'aggregate') { - return tagArtifactKind( - await runSingleCheck('F9', () => ruleF9(artifactDir, artifactName), skipSet), - 'aggregate', - ); + // F23 runs here too: an aggregate artifact pushes its own ETGO_SF_SPEC, so + // its field rows can drift exactly like a window's. ETP-4793 measured 340 of + // the 409 incoherent rows in the live export inside two aggregates + // (return-to-vendor, return-from-customer) — registering F23 for windows + // only would have left 83 % of the defect invisible. + return tagArtifactKind([ + ...await runSingleCheck('F9', () => ruleF9(artifactDir, artifactName), skipSet), + ...await runSingleCheck('F23', () => ruleF23(artifactDir, artifactName, root, goSourcedataDir), skipSet), + ], 'aggregate'); } if (kind === 'aggregate-section') { return tagArtifactKind( - await runAggregateSectionChecks(artifactDir, artifactName, skipSet), + await runAggregateSectionChecks(artifactDir, artifactName, skipSet, root, goSourcedataDir), 'aggregate-section', ); } @@ -1956,6 +2131,10 @@ export async function validatePipeline({ root = ROOT, registryPath, f19AllowlistPath, + // F23 reads the exported ETGO_SF_*.xml from the com.etendoerp.go checkout. + // Injectable so tests point at a fixture instead of whatever module tree + // happens to sit beside the developer's repo. + goSourcedataDir, _artifactsRoot, } = {}) { const artifactsRoot = _artifactsRoot ?? join(root, 'artifacts'); @@ -1982,6 +2161,7 @@ export async function validatePipeline({ skipSet, strict, f19Allowlist, + goSourcedataDir, })); } diff --git a/cli/test/field-visibility.test.js b/cli/test/field-visibility.test.js new file mode 100644 index 000000000..333bf62e8 --- /dev/null +++ b/cli/test/field-visibility.test.js @@ -0,0 +1,97 @@ +import { describe, it } from 'node:test'; +import { strict as assert } from 'node:assert'; +import { + CURATED_VISIBILITIES, + isCuratedVisibility, + mapVisibility, + visibilityMatchesFlags, +} from '../src/lib/field-visibility.js'; +import { mapVisibility as mapVisibilityFromPushToNeo } from '../src/push-to-neo.js'; + +// ETP-4793 — this module is the single source of truth for the curated-visibility +// → NEO-flag projection. It previously existed twice (exported from push-to-neo.js, +// inlined in lib/neo-delta.js) and validator rule F23 would have been a third copy. + +describe('mapVisibility', () => { + it('projects each curated value to its documented flag pair', () => { + assert.deepEqual(mapVisibility('editable'), { isIncluded: 'Y', isReadOnly: 'N' }); + assert.deepEqual(mapVisibility('readOnly'), { isIncluded: 'Y', isReadOnly: 'Y' }); + assert.deepEqual(mapVisibility('system'), { isIncluded: 'Y', isReadOnly: 'Y' }); + assert.deepEqual(mapVisibility('discarded'), { isIncluded: 'N', isReadOnly: 'N' }); + }); + + it('collapses readOnly and system to the same pair', () => { + // The lossy step that makes the VISIBILITY column necessary: the flags alone + // cannot tell an agent which of the two it is looking at. + assert.deepEqual(mapVisibility('readOnly'), mapVisibility('system')); + }); + + it('treats every non-curated value as closed', () => { + for (const value of [null, undefined, '', 'EDITABLE', 'hidden', 0, false]) { + assert.deepEqual(mapVisibility(value), { isIncluded: 'N', isReadOnly: 'N' }, + `expected ${JSON.stringify(value)} to project closed`); + } + }); + + it('is the same function push-to-neo.js exports', () => { + // push-to-neo re-exports it; the name is public API and callers must not get + // a second implementation. + assert.equal(mapVisibilityFromPushToNeo, mapVisibility); + }); +}); + +describe('isCuratedVisibility', () => { + it('accepts exactly the four curated values', () => { + for (const v of CURATED_VISIBILITIES) assert.equal(isCuratedVisibility(v), true, v); + for (const v of [null, undefined, '', 'readonly', 'System']) { + assert.equal(isCuratedVisibility(v), false, JSON.stringify(v)); + } + }); +}); + +describe('visibilityMatchesFlags', () => { + it('accepts a row whose flags match its curated visibility', () => { + const verdict = visibilityMatchesFlags({ visibility: 'readOnly', isIncluded: 'Y', isReadOnly: 'Y' }); + assert.equal(verdict.ok, true); + assert.equal(verdict.kind, 'ok'); + }); + + it('accepts a closed row that carries no visibility', () => { + // N/N IS mapVisibility's default, so an absent VISIBILITY is coherent here. + // The ETP-4793 exclude fix produces exactly these rows. + assert.equal(visibilityMatchesFlags({ isIncluded: 'N', isReadOnly: 'N' }).ok, true); + assert.equal(visibilityMatchesFlags({ visibility: null, isIncluded: 'N', isReadOnly: 'N' }).ok, true); + }); + + it('accepts a row that omits the flags entirely', () => { + assert.equal(visibilityMatchesFlags({}).ok, true); + }); + + it('classifies a curated value disagreeing with the flags as a contradiction', () => { + const verdict = visibilityMatchesFlags({ visibility: 'editable', isIncluded: 'N', isReadOnly: 'N' }); + assert.equal(verdict.ok, false); + assert.equal(verdict.kind, 'contradiction'); + assert.deepEqual(verdict.expected, { isIncluded: 'Y', isReadOnly: 'N' }); + }); + + it('classifies an included row with no visibility as unwritten, not a contradiction', () => { + const verdict = visibilityMatchesFlags({ isIncluded: 'Y', isReadOnly: 'N' }); + assert.equal(verdict.ok, false); + assert.equal(verdict.kind, 'unwritten'); + }); + + it('classifies an unrecognised visibility string as unwritten', () => { + // Not a contradiction: nothing in the pipeline can produce it, so blaming + // the writer would be wrong. It is still incoherent and worth reporting. + assert.equal(visibilityMatchesFlags({ visibility: 'bogus', isIncluded: 'Y', isReadOnly: 'N' }).kind, + 'unwritten'); + }); + + it('catches the readOnly/system pair being stored as editable flags', () => { + // The concrete IMP-26 shape: curation says readOnly, the runtime lets writes + // through because ISREADONLY was never flipped. + const verdict = visibilityMatchesFlags({ visibility: 'readOnly', isIncluded: 'Y', isReadOnly: 'N' }); + assert.equal(verdict.kind, 'contradiction'); + assert.deepEqual(verdict.expected, { isIncluded: 'Y', isReadOnly: 'Y' }); + }); +}); diff --git a/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..a81cf0495 --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..7acffe425 --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_FIELD.xml @@ -0,0 +1,13 @@ + + + + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..0bef5f2d2 --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-aggregate/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..8e2c29ebb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..2364da9c4 --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_FIELD.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c9f66f92f --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..8e2c29ebb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..76265771e --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_FIELD.xml @@ -0,0 +1,57 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c9f66f92f --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..8e2c29ebb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..1a94fd9cb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_FIELD.xml @@ -0,0 +1,24 @@ + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c9f66f92f --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..c528be2bd --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_ENTITY.xml @@ -0,0 +1,3 @@ + + + diff --git a/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..c528be2bd --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_FIELD.xml @@ -0,0 +1,3 @@ + + + diff --git a/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c528be2bd --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-empty/ETGO_SF_SPEC.xml @@ -0,0 +1,3 @@ + + + diff --git a/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..8e2c29ebb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..6c6fb222d --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_FIELD.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c9f66f92f --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_ENTITY.xml b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_ENTITY.xml new file mode 100644 index 000000000..8e2c29ebb --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_ENTITY.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_FIELD.xml b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_FIELD.xml new file mode 100644 index 000000000..7a8224326 --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_FIELD.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_SPEC.xml b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_SPEC.xml new file mode 100644 index 000000000..c9f66f92f --- /dev/null +++ b/cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_SPEC.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/cli/test/validate-pipeline.test.js b/cli/test/validate-pipeline.test.js index b62834399..36b2e241e 100644 --- a/cli/test/validate-pipeline.test.js +++ b/cli/test/validate-pipeline.test.js @@ -31,6 +31,12 @@ async function runOnFixtures(windowNames, opts = {}) { // opts.f19AllowlistPath; otherwise this falls through to the real // (production) allowlist file, which never matches fixture artifact names. f19AllowlistPath: opts.f19AllowlistPath, + // F23 reads the exported ETGO_SF_*.xml from a com.etendoerp.go checkout. + // Default every test at a fixture holding valid-but-empty XMLs: without + // this, F23 would resolve the conventional `/../modules/...` path and + // its verdict would depend on whether the developer happens to have the + // runtime module checked out next to this repo. F23's own tests override it. + goSourcedataDir: opts.goSourcedataDir ?? join(FIXTURES, '..', 'f23-sourcedata-empty'), // Override the artifacts root to point at our fixture directory _artifactsRoot: FIXTURES, }); @@ -1110,6 +1116,103 @@ describe('Rule F22 — customTabsAfterBottom + tabOrder', () => { }); }); +// ─── F23 — ETGO_SF_FIELD.VISIBILITY vs the flags it projects to ──────────── +// ETP-4793 / IMP-26 §5.3. Every fixture below declares its spec NAME as +// 'window-ok' so it lines up with an existing window artifact — F23 keys the +// XML rows by spec name, and the artifact directory name IS the spec name. + +const F23_FIXTURES = join(FIXTURES, '..'); + +function runF23(sourcedataFixture) { + return runOnFixtures(['window-ok'], { + goSourcedataDir: sourcedataFixture === null ? join(F23_FIXTURES, 'f23-no-such-dir') : join(F23_FIXTURES, sourcedataFixture), + }); +} + +describe('Rule F23 — pushed visibility vs ISINCLUDED/ISREADONLY', () => { + it('BLOCK when a stored VISIBILITY contradicts the flags it projects to', async () => { + const result = await runF23('f23-sourcedata-contradiction'); + const f23 = result.violations.find(v => v.rule === 'F23'); + assert.ok(f23, 'F23 should fire on a curated visibility that disagrees with the flags'); + assert.equal(f23.severity, 'BLOCK'); + assert.match(f23.message, /header\.documentNo/); + assert.match(f23.message, /visibility='editable'/); + assert.equal(f23.f23Contradictions, 1); + }); + + it('reports the contradiction rather than the unwritten row when both exist', async () => { + // The same fixture also holds an included row with no VISIBILITY. A rule + // returns one entry, so the blocking class must win — otherwise a real + // contradiction hides behind pre-existing backfill debt. + const result = await runF23('f23-sourcedata-contradiction'); + const f23 = result.violations.find(v => v.rule === 'F23'); + assert.equal(f23.severity, 'BLOCK'); + assert.equal(f23.f23Unwritten, 1, 'the unwritten row is still counted, just not the headline'); + assert.ok(!/orderDate/.test(f23.message), 'the WARN-class row must not be listed as a contradiction'); + }); + + it('WARN when included rows carry no VISIBILITY at all', async () => { + const result = await runF23('f23-sourcedata-unwritten'); + const f23 = result.violations.find(v => v.rule === 'F23'); + assert.ok(f23, 'F23 should report the backfill gap'); + assert.equal(f23.severity, 'WARN'); + assert.equal(f23.f23Unwritten, 2); + assert.match(f23.message, /header\.orderDate/); + assert.match(f23.message, /header\.businessPartner/); + assert.ok(!/documentNo/.test(f23.message), 'the coherent editable row must not be reported'); + }); + + it('passes when every row projects correctly, including a closed row with no VISIBILITY', async () => { + // N/N with no VISIBILITY is exactly mapVisibility's default, so it is + // coherent — the ETP-4793 exclude fix relies on that and must stay green. + const result = await runF23('f23-sourcedata-clean'); + assert.ok(!result.violations.find(v => v.rule === 'F23'), 'F23 must not fire on coherent rows'); + }); + + it('ignores rows flagged ISACTIVE=N', async () => { + const result = await runF23('f23-sourcedata-inactive'); + assert.ok(!result.violations.find(v => v.rule === 'F23'), 'retired config is not a live surface'); + }); + + it('is inert — not skipped — without a com.etendoerp.go checkout', async () => { + const result = await runF23(null); + assert.ok(!result.violations.find(v => v.rule === 'F23'), 'no violation without the XML'); + assert.ok(!result.skipped.find(s => s.rule === 'F23'), + 'and no skipped entry either: one per artifact would drown the report on a functional-only checkout'); + }); + + it('also runs on aggregate artifacts, which push their own spec', async () => { + // The regression this pins: F23 was first registered for kind === 'window' + // only and reported 69 of the 409 incoherent rows in the live export — the + // other 340 sat in two aggregate artifacts (return-to-vendor, + // return-from-customer) that the rule never visited. + const result = await runOnFixtures(['aggregate-ok'], { + goSourcedataDir: join(F23_FIXTURES, 'f23-sourcedata-aggregate'), + }); + const f23 = result.violations.find(v => v.rule === 'F23'); + assert.ok(f23, 'F23 must visit aggregate artifacts'); + assert.equal(f23.severity, 'BLOCK'); + assert.equal(f23.artifactKind, 'aggregate'); + assert.match(f23.message, /lines\.returnQty/); + }); + + it('labels a row with no AD_COLUMN_ID by primary key, not "undefined"', async () => { + const result = await runF23('f23-sourcedata-blank'); + const f23 = result.violations.find(v => v.rule === 'F23'); + assert.ok(f23); + assert.ok(!/\.undefined/.test(f23.message), 'an unidentifiable field makes the warning useless'); + assert.match(f23.message, /no AD_COLUMN_ID, field F0{30}1/); + }); + + it('honours --skip=F23', async () => { + const result = await runOnFixtures(['window-ok'], { + goSourcedataDir: join(F23_FIXTURES, 'f23-sourcedata-contradiction'), + skip: ['F23'], + }); + assert.ok(!result.violations.find(v => v.rule === 'F23')); + }); +}); + // ─── F19 — custom table `required` flag drift (ETP-4609 follow-up) ───────── // These fixtures need a real tools/app-shell/src/windows/custom// tree // to resolve against, so `root` is overridden to the fixtures dir itself (the diff --git a/docs/pipeline-validator-reference.md b/docs/pipeline-validator-reference.md index 270ed06a2..f13791564 100644 --- a/docs/pipeline-validator-reference.md +++ b/docs/pipeline-validator-reference.md @@ -63,6 +63,7 @@ Rules are grouped by the artifact kind they apply to (see [Artifact Classificati | F17 | BLOCK | `decisions.json` `window.balanceFooter` is missing `debitField`/`creditField`, or references a field that does not exist on the lines entity (validated against `frontendContract.entities..fields[].name`). | Set `window.balanceFooter` to `{ debitField, creditField }` using amount-typed line-entity field names that exist in the contract. | | F21 | BLOCK | The declared read-only intent in `decisions.json` (`window.readOnly`, `entities..readOnly`, `entities..methods`, and — since ETP-4745 — `window.hideDelete` / `entities..hideDelete`) does not match the HTTP methods the contract would push to `ETGO_SF_ENTITY` — i.e. `apiPrediction.crud..methods` / `apiPrediction.window.readOnly` / `apiPrediction.crud..delete` is stale. Also fires when an emitted `crud..methods` array omits `GET`/`GETBYID` (an entity with no read access is never valid), or when a restricting `entities.` declaration (including a `hideDelete`-only one) matches no contract entity (mistyped key → silent no-op). | Regenerate the contract (`make regen ONLY=`). Both write paths — the live DB push and the offline XML delta — read the method flags off the contract, so a stale contract silently re-opens a read-only window (or re-enables DELETE on a hideDelete entity) for writes on the next push (ETP-4254 / ETP-4745). Never hand-edit `crud.methods`. | | F22 | BLOCK | `decisions.json` sets `window.customTabsAfterBottom: true` (custom tabs render in a strip below `bottomSection`, outside the sorted tab list) while a custom tab in `customPanelTabs`/`extraTabs`/`attachments` still declares a `tabOrder` — that `tabOrder` is a silent no-op. | Remove `window.customTabsAfterBottom`, or remove `tabOrder` from the listed custom tab(s) (ETP-4415). | +| F23 | BLOCK / WARN | A pushed `ETGO_SF_FIELD` row whose `VISIBILITY` does not project to its own `ISINCLUDED`/`ISREADONLY` pair under `mapVisibility()` (`lib/field-visibility.js`). The curated decision is stored twice on purpose — the runtime enforces the booleans, `neo_schema` reports the word, and the booleans cannot recover it (`readOnly` and `system` both collapse to `Y`/`Y`) — so the copies can disagree. **BLOCK** when `VISIBILITY` holds a curated value projecting to a different pair (writer bug or hand-edit); **WARN** when `VISIBILITY` is absent while `ISINCLUDED='Y'` (backfill debt: agents cannot tell `readOnly` from `system`). An absent `VISIBILITY` on a closed row is coherent, not a violation — `N`/`N` *is* `mapVisibility(null)`. Ignores `ISACTIVE='N'`. Reads the exported sourcedata XML from `${SF_GO_SOURCEDATA_DIR}` → `${ETENDO_ROOT}/modules/com.etendoerp.go/src-db/database/sourcedata` → the sibling `../modules/...`; **inert (no violation, no `skipped` entry) when that checkout is absent**, so a green F23 is not by itself evidence the rule ran. Registered for `window` and `aggregate` artifacts. | Re-push (`make regen ONLY= PUSH_TO_NEO=1`) then `./gradlew export.database`. A BLOCK surviving a clean push is a writer bug — fix `neo-writer.js → populateSpec`, never the XML (ETP-4793 / IMP-26 §5.3). | ### Report rules @@ -75,6 +76,7 @@ Rules are grouped by the artifact kind they apply to (see [Artifact Classificati | Code | Severity | What it detects | How to fix | |------|----------|-----------------|------------| | F9 | BLOCK | Aggregate folder has `generated/` but no `aggregate-contract.json`. | Re-run the pipeline for the aggregate, or add the missing `aggregate-contract.json`. | +| F23 | BLOCK / WARN | Same rule as in the window table — an aggregate pushes its own `ETGO_SF_SPEC`, so its field rows drift identically. At ETP-4793, 340 of the 409 incoherent rows in the live export were inside two aggregates, so registering F23 for windows alone would have hidden 83 % of the defect. | See F23 in the window table. | ### General rules From 2c184c702be6776373eb2a7caa8552ea4c1c88d3 Mon Sep 17 00:00:00 2001 From: Valentin Vivaldi Date: Thu, 13 Aug 2026 10:04:10 -0300 Subject: [PATCH 6/6] Feature ETP-4793: Reorder default params to satisfy Sonar S1788 ruleF23 and runWindowChecks had a defaulted parameter followed by a non-defaulted one. Reordered the signatures and updated all internal call sites. Behavior unchanged. --- cli/src/validate-pipeline.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cli/src/validate-pipeline.js b/cli/src/validate-pipeline.js index 0ce5c3be1..3f718501c 100644 --- a/cli/src/validate-pipeline.js +++ b/cli/src/validate-pipeline.js @@ -914,11 +914,11 @@ function f23FieldLabel({ row, entityName }) { * * @param {string} artifactDir * @param {string} artifactName - artifact dir name === spec name - * @param {string} root * @param {string} [sourcedataDir] + * @param {string} [root] * @returns {Promise} */ -async function ruleF23(artifactDir, artifactName, root = ROOT, sourcedataDir) { +async function ruleF23(artifactDir, artifactName, sourcedataDir, root = ROOT) { const dir = sourcedataDir ?? resolveGoSourcedataDir(root); const bySpec = await loadF23FieldsBySpec(dir); if (bySpec === null) return null; // no runtime-module checkout — nothing to read @@ -1964,7 +1964,7 @@ async function runEnabledChecks(checks, skipSet) { return (await Promise.all(pendingChecks)).filter(Boolean); } -async function runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist = [], goSourcedataDir) { +async function runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, goSourcedataDir, f19Allowlist = []) { return runEnabledChecks([ { rule: 'F1', run: () => ruleF1(artifactDir, artifactName) }, { rule: 'F2', run: () => ruleF2(artifactDir, artifactName) }, @@ -1986,7 +1986,7 @@ async function runWindowChecks(artifactDir, artifactName, registryContent, root, { rule: 'F20', run: () => ruleF20(artifactDir, artifactName, root) }, { rule: 'F21', run: () => ruleF21(artifactDir, artifactName) }, { rule: 'F22', run: () => ruleF22(artifactDir, artifactName) }, - { rule: 'F23', run: () => ruleF23(artifactDir, artifactName, root, goSourcedataDir) }, + { rule: 'F23', run: () => ruleF23(artifactDir, artifactName, goSourcedataDir, root) }, ], skipSet); } @@ -2004,7 +2004,7 @@ async function runAggregateSectionChecks(artifactDir, artifactName, skipSet, roo const f9Results = await runSingleCheck('F9', () => ruleF9(artifactDir, artifactName), skipSet); const f4Results = await runSingleCheck('F4', () => ruleF4(artifactDir, artifactName), skipSet); const f23Results = await runSingleCheck( - 'F23', () => ruleF23(artifactDir, artifactName, root, goSourcedataDir), skipSet, + 'F23', () => ruleF23(artifactDir, artifactName, goSourcedataDir, root), skipSet, ); return [...f9Results, ...f4Results, ...f23Results]; } @@ -2012,7 +2012,7 @@ async function runAggregateSectionChecks(artifactDir, artifactName, skipSet, roo async function runChecksForArtifact({ kind, artifactDir, artifactName, registryContent, root, skipSet, strict, f19Allowlist, goSourcedataDir }) { if (kind === 'window') { return tagArtifactKind( - await runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, f19Allowlist, goSourcedataDir), + await runWindowChecks(artifactDir, artifactName, registryContent, root, skipSet, goSourcedataDir, f19Allowlist), 'window', ); } @@ -2030,7 +2030,7 @@ async function runChecksForArtifact({ kind, artifactDir, artifactName, registryC // only would have left 83 % of the defect invisible. return tagArtifactKind([ ...await runSingleCheck('F9', () => ruleF9(artifactDir, artifactName), skipSet), - ...await runSingleCheck('F23', () => ruleF23(artifactDir, artifactName, root, goSourcedataDir), skipSet), + ...await runSingleCheck('F23', () => ruleF23(artifactDir, artifactName, goSourcedataDir, root), skipSet), ], 'aggregate'); } if (kind === 'aggregate-section') {