Feature ETP-4793: Persist field visibility, F23 validator rule - #128
Feature ETP-4793: Persist field visibility, F23 validator rule#128valenvivaldi wants to merge 11 commits into
Conversation
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.
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.
`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.
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.
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.
ruleF23 and runWindowChecks had a defaulted parameter followed by a non-defaulted one. Reordered the signatures and updated all internal call sites. Behavior unchanged.
Copilot PR ReviewOutcome: Request changes Blocking findings
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot PR Review
Outcome: Request changes
Blocking findings must be resolved before merge.
Blocking findings
-
Duplicated added block (
DUPLICATED_BLOCK)
The same normalized block of 10 added lines appears in multiple places. Extract shared logic or remove the copy-paste. -
cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_ENTITY.xml:1-10 -
cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_ENTITY.xml:1-10 -
cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_ENTITY.xml:1-10 -
cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_ENTITY.xml:1-10 -
cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_ENTITY.xml:1-10 -
Duplicated added block (
DUPLICATED_BLOCK)
The same normalized block of 9 added lines appears in multiple places. Extract shared logic or remove the copy-paste. -
cli/test/fixtures/f23-sourcedata-blank/ETGO_SF_SPEC.xml:1-9 -
cli/test/fixtures/f23-sourcedata-clean/ETGO_SF_SPEC.xml:1-9 -
cli/test/fixtures/f23-sourcedata-contradiction/ETGO_SF_SPEC.xml:1-9 -
cli/test/fixtures/f23-sourcedata-inactive/ETGO_SF_SPEC.xml:1-9 -
cli/test/fixtures/f23-sourcedata-unwritten/ETGO_SF_SPEC.xml:1-9
|
0 New Issues
0 Fixed Issues
0 Accepted Issues1 participant
Tooling-side (published packages) support for the field-visibility and entity-exclusion fixes surfaced by the MCP benchmark, plus preview-publishing pipeline hardening.
cli/src/neo-writer.js,cli/src/push-to-neo.js,cli/src/lib/field-visibility.js)exclude:trueasISINCLUDED='N'in NEO (cli/src/lib/entity-methods.js,cli/src/lib/neo-delta.js); explicit comparator for the excluded-entity sortcli/src/validate-pipeline.js) with fixture matrix (clean/blank/aggregate/contradiction/inactive/unwritten/empty sourcedata).github/workflows/publish-preview.yml)field-visibility.test.js,neo-writer-populate.test.js,neo-writer-upsert-field.test.js,push-to-neo-helpers.test.js,validate-pipeline.test.js,entity-methods-pipeline.test.jsdocs/decisions-reference.mdanddocs/pipeline-validator-reference.mdfor F23Jira
ETP-4793, epic ETP-3504