fix(model): omit join-key columns instead of hiding them (ISS-8 follow-up)

The Aug 24 naming fix (3986a1e) marked surrogate/FK *_KEY columns
is_hidden: true, but live verification on TRI_08250424_PY3_mdl showed
hidden keys still display their raw junk names ("Dim Dim Broker Key")
in the ThoughtSpot model editor page — is_hidden only hides from
search/Spotter, not the editor stakeholders actually look at.

Fix: omit *_KEY columns from the model's columns list entirely in
_create_model_with_constraints(). Joins are unaffected since they're
defined at the table level (model_tables[].joins reference physical
TABLE::COLUMN names directly, independent of the columns list). All
liveboard tiles on the Triumph model confirmed still resolve with keys
fully removed.

New coverage: tests/test_model_key_column_omission.py.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

sprint_2026_04.md

@@ -259,7 +259,7 @@ should tell. KPI targets, growth trends, and anomaly patterns live in the matrix
 - [x] **Model-quality fixes from TRI_08190447_DXO review (Aug 24)** — investigated mani's Triumph model (sebe); found four systemic pipeline defects and fixed all in code (NOT applied to the existing TRI objects — that demo stays as-is per boone):
   - **Join spanning-tree bug**: `_remove_diamond_joins()` reduced every model to N−1 joins via undirected union-find, silently dropping legitimate fact→dim joins in galaxy schemas (TRI lost FACT_BROKER_PAYMENT→DIM_BROKER and FACT_CARRIER_CAPACITY→DIM_LANE — payment metrics unsliceable by broker). Rewritten to directed-reachability: only prunes true diamonds (two paths between same pair) and directed cycles; shared conformed dims are kept ✅
   - **DIM_/FACT_ naming banned end-to-end**: blueprint prompt rule 2a + `_table_ident()` normalization in `blueprint.py` (strips prefixes even when the user request includes them; directives matching already prefix-tolerant); Example B updated. Deployer `_strip_dim_fact_prefix()` strips prefixes from display names; conflict resolution now gives the home table the clean name ("Broker Key") and others a readable suffix ("Broker Key (Broker Payment)") — no more "Dim Dim Broker Key" / "fact_Fact …" / "_2" ✅
-  - **Key columns hidden**: surrogate PKs + FKs get `is_hidden: true` in model TML (still present for joins) ✅
+  - **Key columns omitted (updated Aug 25)**: surrogate PKs + FKs were initially given `is_hidden: true` in model TML, but live verification on `TRI_08250424_PY3_mdl` showed hidden keys still display their raw junk names ("Dim Dim Broker Key") in the ThoughtSpot model editor — `is_hidden` only hides from search/Spotter. Fix updated to omit `*_KEY` columns from the model's `columns` list entirely; joins are unaffected since they're defined at the table level (`model_tables[].joins` reference physical `TABLE::COLUMN` names, independent of the columns list). All liveboard tiles confirmed still resolve. New coverage: `tests/test_model_key_column_omission.py` ✅
   - **Column classification**: token-based matching in `_determine_column_type()` — INVOICES_PAID no longer an ATTRIBUTE (endswith-'ID' trap), CORPORATE/GENERATED no longer match 'RATE'; RATIO/PCT/DAYS_TO_* now AVERAGE instead of SUM ✅
   - Verified offline against the exact TRI structure: `scratch/verify_fixes.py` — 25/25 pass. Repaired TRI TMLs staged in scratch for reference only (`model_tri_fixed.tml.yml`, `lb_*_fixed.tml.yml`), not imported.
 

tests/test_model_key_column_omission.py

@@ -0,0 +1,97 @@
+"""
+Regression test for the join-key visibility fix on deploy/mcp (2026-08-25).
+
+_create_model_with_constraints() previously marked surrogate/FK *_KEY columns
+is_hidden: true. That hides them from search/Spotter but they still show up
+with their raw physical names ("Dim Dim Broker Key") in the ThoughtSpot model
+editor page — verified live on TRI_08250424_PY3_mdl, the surface stakeholders
+actually look at. Fix: omit *_KEY columns from the model's columns list
+entirely. Joins stay intact because they're defined at the table level
+(model_tables[].joins reference physical TABLE::COLUMN names directly).
+"""
+import re
+
+import pytest
+import yaml
+
+import thoughtspot_deployer as tsd
+
+
+@pytest.fixture
+def deployer(monkeypatch):
+    monkeypatch.setattr(
+        tsd, 'get_admin_setting',
+        lambda key, required=True: f'test-{key.lower()}'
+    )
+    return tsd.ThoughtSpotDeployer(
+        base_url='https://unit-test.thoughtspot.cloud',
+        username='unit-test@example.com',
+        secret_key='unit-test-key',
+    )
+
+
+def _star_schema():
+    tables = {
+        'DIM_BROKER': [
+            {'name': 'DIM_BROKER_KEY', 'type': 'NUMBER(38,0)'},
+            {'name': 'DIM_BROKER_NAME', 'type': 'VARCHAR(120)'},
+            {'name': 'BROKER_TIER', 'type': 'VARCHAR(20)'},
+        ],
+        'FACT_LOAD_TRANSACTION': [
+            {'name': 'FACT_LOAD_TRANSACTION_KEY', 'type': 'NUMBER(38,0)'},
+            {'name': 'DIM_BROKER_KEY', 'type': 'NUMBER(38,0)'},
+            {'name': 'LOAD_DATE', 'type': 'DATE'},
+            {'name': 'TOTAL_LOAD_REVENUE_USD', 'type': 'NUMBER(18,2)'},
+        ],
+    }
+    foreign_keys = [
+        {'from_table': 'FACT_LOAD_TRANSACTION', 'from_column': 'DIM_BROKER_KEY',
+         'to_table': 'DIM_BROKER', 'to_column': 'DIM_BROKER_KEY'},
+    ]
+    table_guids = {name: f'guid-{name.lower()}' for name in tables}
+    return tables, foreign_keys, table_guids
+
+
+def _build_model(deployer):
+    tables, foreign_keys, table_guids = _star_schema()
+    tml = deployer._create_model_with_constraints(
+        tables, foreign_keys, table_guids,
+        table_constraints={}, model_name='UNIT_TEST_mdl',
+        connection_name='unit_test_conn',
+    )
+    return yaml.safe_load(tml)
+
+
+def test_join_key_columns_are_omitted_not_hidden(deployer):
+    model = _build_model(deployer)
+    columns = model['model']['columns']
+
+    key_backed = [c for c in columns if c['column_id'].upper().endswith('_KEY')]
+    assert key_backed == [], f"*_KEY columns must not appear in the model at all: {key_backed}"
+
+    hidden = [c for c in columns if c.get('properties', {}).get('is_hidden')]
+    assert hidden == [], "is_hidden is no longer how keys are handled — they should be absent, not hidden"
+
+    key_named = [c for c in columns if re.search(r'\bkey\b', c['name'], re.IGNORECASE)]
+    assert key_named == [], f"No column should be named like a key: {key_named}"
+
+    # 7 source columns minus 3 keys = 4 business columns
+    assert len(columns) == 4
+
+
+def test_joins_still_reference_physical_key_columns(deployer):
+    """Keys leave the columns list but must stay in the table-level join clauses."""
+    model = _build_model(deployer)
+    fact = next(t for t in model['model']['model_tables']
+                if t['name'] == 'FACT_LOAD_TRANSACTION')
+    on_clauses = [j.get('on', j.get(True)) for j in fact['joins']]
+
+    assert '[FACT_LOAD_TRANSACTION::DIM_BROKER_KEY] = [DIM_BROKER::DIM_BROKER_KEY]' in on_clauses
+
+
+def test_business_columns_unaffected(deployer):
+    model = _build_model(deployer)
+    names = {c['name'] for c in model['model']['columns']}
+    assert 'Broker Tier' in names
+    assert 'Load Date' in names
+    assert 'Total Load Revenue Usd' in names

thoughtspot_deployer.py

@@ -1403,19 +1403,30 @@ class ThoughtSpotDeployer:
         # Add columns with proper global conflict resolution (same as working version)
         used_display_names = set()
 
-        # Key columns (surrogate PKs and FKs) must be IN the model for joins to
-        # work, but they're join plumbing, not demo content — mark them hidden.
+        # Key columns (surrogate PKs and FKs) are needed in the physical tables
+        # for joins, but joins are defined at the table level (model_tables[].joins
+        # reference TABLE::COLUMN directly) — the model's columns list doesn't need
+        # them. Previously these were kept with is_hidden: true, but that still
+        # left the junk names ("Dim Dim Broker Key") visible in the ThoughtSpot
+        # model editor — is_hidden only hides from search/Spotter. Verified live
+        # on TRI_08250424_PY3_mdl (2026-08-25): every liveboard tile still
+        # resolves with all key columns omitted entirely.
         fk_columns = set()
         for fk in foreign_keys or []:
             fk_columns.add((fk.get('from_table', '').upper(), fk.get('from_column', '').upper()))
             fk_columns.add((fk.get('to_table', '').upper(), fk.get('to_column', '').upper()))
 
+        omitted_keys = []
         for table_name, columns in tables.items():
             table_name_upper = table_name.upper()
             for col in columns:
                 col_name = col['name'].upper()
                 original_col_name = col.get('original_name', col['name'])  # Use original casing for display
 
+                if col_name.endswith('_KEY') or (table_name_upper, col_name) in fk_columns:
+                    omitted_keys.append(f"{table_name_upper}.{col_name}")
+                    continue
+
                 # Start with basic conflict resolution
                 display_name = self._resolve_column_name_conflict(
                     col_name, table_name_upper, column_name_counts,
@@ -1453,11 +1464,6 @@ class ThoughtSpotDeployer:
                 if aggregation:
                     column_def['properties']['aggregation'] = aggregation
 
-                # Hide surrogate/foreign key columns — required for joins,
-                # but nobody demos "broker key 23455"
-                if col_name.endswith('_KEY') or (table_name_upper, col_name) in fk_columns:
-                    column_def['properties']['is_hidden'] = True
-
                 # Add calendar property for DATE columns so ThoughtSpot enables
                 # time bucketing (.weekly, .monthly, etc.) on them
                 if self._map_data_type(col['type']) == 'DATE':
@@ -1465,6 +1471,9 @@ class ThoughtSpotDeployer:
 
                 model['model']['columns'].append(column_def)
 
+        if omitted_keys:
+            print(f"   🙈 Omitted {len(omitted_keys)} join-key columns from model: {', '.join(omitted_keys)}")
+
         # Generate YAML output with validation
         yaml_output = yaml.dump(model, default_flow_style=False, sort_keys=False,
                                default_style=None, indent=2, width=120)