[feature/capgen-v1] Fix three correctness findings from PR #762 review (@jimmielin/claude)

capgen/generator/suite_resolver.py

@@ -1824,6 +1824,21 @@ def _resolve_one_arg(
         and any(_dim_has_vertical(d) for d in host_dims)
     )
 
+    # A vertical flip is realised by a reverse-stride subscript on the host
+    # side.  An allocatable actual argument must omit explicit subscripts
+    # (handled just below), so the flip cannot be encoded.
+    if needs_vert_flip and host_allocatable:
+        raise CCPPError(
+            "Variable '{}' (standard_name='{}'): {} is allocatable and also "
+            "requires a vertical flip (top_at_one differs between the host and "
+            "scheme '{}'), but a vertical flip cannot be applied to an "
+            "allocatable actual argument -- its subscripts must be omitted.  "
+            "Make top_at_one agree between the host and scheme metadata, or "
+            "declare the host variable non-allocatable.".format(
+                local, std_name, source, scheme_name
+            )
+        )
+
     if host_allocatable:
         # Allocatable actual arguments must omit explicit dimension ranges:
         # the callee declares the dummy as allocatable too and assumes the
@@ -1906,6 +1921,25 @@ def _resolve_one_arg(
     else:
         needs_kind = bool(host_kind) and bool(scheme_kind) and host_kind != scheme_kind
 
+    needs_transform = needs_unit or needs_kind or needs_vert_flip
+
+    # ---- local variable names (transformation temp + pointer) ------------
+    # Resolve the temp/pointer local names BEFORE building the transform
+    # expressions below.  The backward expression references the temp by
+    # name, and ``_local_name_conflict`` may rename it away from a colliding
+    # scheme local (e.g. a scheme that declares its own ``<name>_l``).
+    temp_name = ''
+    ptr_name  = ''
+    if needs_transform:
+        candidate = '{}_l'.format(local)
+        temp_name = _local_name_conflict(candidate, used_local_names)
+        used_local_names.add(temp_name.lower())
+
+    if optional:
+        candidate = '{}_p'.format(local)
+        ptr_name = _local_name_conflict(candidate, used_local_names)
+        used_local_names.add(ptr_name.lower())
+
     # Forward transformation expression (host/suite → scheme local).
     # ``call_expr`` already carries the flipped vertical subscript when
     # ``needs_vert_flip`` is True, so the unit-conversion formula naturally
@@ -1931,32 +1965,14 @@ def _resolve_one_arg(
     # Backward transformation expression (scheme local → host/suite).
     unit_backward = ''
     if needs_unit and bwd_fn is not None and intent in ('out', 'inout'):
-        unit_backward_expr = '{}_l'.format(local)
-        unit_backward = _apply_transform_formula(bwd_fn, unit_backward_expr, host_kind)
+        unit_backward = _apply_transform_formula(bwd_fn, temp_name, host_kind)
     elif needs_kind and intent in ('out', 'inout'):
         unit_backward = _kind_cast_expr(
-            scheme_var.type, '{}_l'.format(local), host_kind,
+            scheme_var.type, temp_name, host_kind,
             local=local, std_name=std_name, scheme_name=scheme_name,
         )
     elif needs_vert_flip and not needs_unit and intent in ('out', 'inout'):
-        unit_backward = '{}_l'.format(local)
-
-    needs_transform = needs_unit or needs_kind or needs_vert_flip
-
-    # ---- local variable names (transformation temp + pointer) ------------
-    # ``used_local_names`` stores the LOWERCASED names so collision
-    # detection is Fortran-case-insensitive (see _local_name_conflict).
-    temp_name = ''
-    ptr_name  = ''
-    if needs_transform:
-        candidate = '{}_l'.format(local)
-        temp_name = _local_name_conflict(candidate, used_local_names)
-        used_local_names.add(temp_name.lower())
-
-    if optional:
-        candidate = '{}_p'.format(local)
-        ptr_name = _local_name_conflict(candidate, used_local_names)
-        used_local_names.add(ptr_name.lower())
+        unit_backward = temp_name
 
     # ---- transform case --------------------------------------------------
     if optional and needs_transform:

capgen/metadata/metadata_table.py

@@ -1296,9 +1296,24 @@ def flush_section(lineno: int) -> None:
         current_section = None
 
     def flush_table_props() -> None:
-        """Apply any extra table-property keys to the current table."""
-        if current_table is not None and collecting_table_props:
-            current_table.apply_table_props(pending_props)
+        """Apply extra table-property keys, or reject an incomplete block.
+
+        A ``[ccpp-table-properties]`` that is missing the ``type`` and/or
+        ``name`` key is invalid; raise a clear error here. An unknown
+        ``type`` *value* is rejected separately when the table is
+        built (see :class:`MetadataTable`).
+        """
+        if not collecting_table_props:
+            return
+        if current_table is None:
+            missing = [k for k in ('name', 'type') if k not in pending_props]
+            raise CCPPError(
+                "[ccpp-table-properties] block at {} is missing required "
+                "attribute(s): {}".format(
+                    ctx(pending_start), ', '.join(missing)
+                )
+            )
+        current_table.apply_table_props(pending_props)
 
     for lineno, raw_line in enumerate(lines):
         line = raw_line.rstrip('\n').rstrip('\r')

doc/followups.md

@@ -48,6 +48,7 @@ Status values: `open`, `in progress`, `blocked`, `closed`.
 | FU-030 | Deterministic + documented constituent registration order in the generator | framework | 2026-06-11 | open | Root cause of the cam4 FWAUT b4b diff (the framework side of FU-018): capgen-ng registers water species alphabetically ([cloud_ice, cloud_liquid, water_vapor]) vs original's declaration order ([cloud_liquid, cloud_ice, water_vapor]), and trace gases differ too, so `air_composition`'s `thermodynamic_active_species_idx` order → `get_hydrostatic_energy` water-sum FP order → energy fixer → pervasive roundoff.  Proven b4b by a flag-guarded reorder hack.  **Decision (Dom): RE-BASELINE** — give capgen-ng a deterministic, documented order (qv first; an understandable rule for how constituents land in the array), then CAM-SIMA re-baselines against the original-capgen reference; not match-the-old-order.  Levers: `host_constituents.py` / the legacy-auto-clone path (FU-012) / `ccpp_register_constituents` emission; intersects the constituents overhaul (FU-020).  Analysis: `doc/cam4_fwaut_constituent_order.md`. |
 | FU-031 | Long-term redesign of the `ccpp_static_api.F90` runtime listings | framework | 2026-05-14 | open | The suite-variable / suite-host-data listings made the introspection module ~33k lines (`-O3` effectively hangs).  Immediate pressure is off — `--no-host-introspection` stubs them (→ ~800 lines) — so this is **no longer blocking**, but the long-term redesign stays open for team discussion: move the listings to a runtime read of `datatable.xml` (preferred — no recompile when listings change), or a separate `-O0` file, or static string `data` tables, or lazy-emit only the routines the host calls.  Do not redesign unilaterally. |
 | FU-032 | Generator-owned locals can silently shadow a host import — auto-uniquify | framework | 2026-08-07 | open | `_check_host_control_local_collisions` (`capgen/generator/group_cap.py`) now hard-errors when a host variable's local name collides with a control-variable dummy (issue #774 — the silent wrong-value case, closed by that check + `unit-tests/test_suite_resolver.py::TestHostControlLocalNameCollision`).  Two other subroutine-scope locals can shadow a use-associated host import the same way but are **generator-owned**, so the right fix is to rename *them*, not error: transformation temporaries (`<name>_l` / `<name>_p`) and subcycle loop counters (`ccpp_loop_counter*`).  Seed the temp uniquifier (`used_local_names_phase`, `suite_resolver.py:2446`) with the group's host-import symbols + control-dummy names so `_local_name_conflict` renames generator locals away from them.  Rare in practice (suffixed/reserved names) but closes the class.  Deliberately deferred out of the #774 fix (Step 2, 2026-08-07). |
+| FU-033 | Vertical flip on an allocatable host array is rejected, not supported | framework | 2026-08-12 | open | `_resolve_one_arg` (`capgen/generator/suite_resolver.py`, just after `needs_vert_flip` is computed) hard-errors when a host variable is `allocatable = True` **and** needs a vertical flip (host/scheme `top_at_one` disagree on a var with a vertical dim).  An allocatable actual must omit subscripts, so the reverse-stride flip subscript cannot be encoded; silently dropping it would hand the scheme vertically-reversed data.  **Decision (2026-08-12, PR #762 review finding from jimmielin): error for now** rather than emit a wrong-but-compiling cap.  Test: `unit-tests/test_suite_resolver.py::TestVerticalFlipTransform::test_allocatable_host_plus_flip_raises`.  Any future support is limited to the allocatable-host → *non-allocatable* (plain assumed-shape) scheme-dummy sub-case, where a flipped section `host(:, ub:lb:-1)` is legal; an allocatable dummy can never receive a flipped section.  Parallels FU-032 (guard now, enhance later). |
 
 ---
 
@@ -146,6 +147,7 @@ file, per the procedure in the repository's `CLAUDE.md`.
 
 | Machine | Last reconciled | By |
 |---------|-----------------|-----|
+| `dutchman` | 2026-08-12 | PR #762 review-fix session (jimmielin's Claude-generated findings): added FU-033 (vertical-flip + allocatable-host → hard error).  The other two findings — backward-transform temp-name collision (`suite_resolver`) and incomplete `[ccpp-table-properties]` missing `name`/`type` now erroring (`metadata_table`) — are fixed, committed, and PR'd, so they live in git, not restated here.  Nothing new in this machine's auto-memory to fold beyond the above. |
 | `dutchman` | 2026-08-07 | folded the issue #772 / #774 session: added FU-032 (generator-local shadow follow-up).  #772 shown to be a non-issue in v1 (cld_shadow e2e reproducer) and #774 detect-and-error landed in `group_cap.py` — both tracked in GitHub, not restated here |
 | `dutchman` | 2026-08-06 | first sweep of this machine; folded its auto-memory investigation notes into new rows FU-025…FU-031, added Codee `use…only:` detail to FU-005, cross-linked FU-018↔FU-030 |
 | `ip-10-0-0-98.ec2.internal` | 2026-07-29 | swept on adding FU-024; local stores unchanged since the previous sweep, nothing new to fold in |

unit-tests/test_metadata_table.py

@@ -918,6 +918,91 @@ def test_banana_type_rejected(self):
         with self.assertRaises(CCPPError):
             _parse_text(text)
 
+    def test_missing_type_in_table_properties_rejected(self):
+        """A [ccpp-table-properties] block with no ``type`` must raise, naming
+        the missing attribute -- not silently drop the table or fail later
+        with a misleading 'variable outside any section' error.
+        """
+        text = """
+            [ccpp-table-properties]
+              name = my_host
+
+            [ccpp-arg-table]
+              name = my_host
+              type = host
+            [ im ]
+              standard_name = horizontal_dimension
+              units = count
+              dimensions = ()
+              type = integer
+        """
+        with self.assertRaises(CCPPError) as cm:
+            _parse_text(text)
+        self.assertIn('type', str(cm.exception))
+
+    def test_mistyped_type_key_in_table_properties_rejected(self):
+        """A typo in the ``type`` key (``tpye``) leaves the block with no
+        recognised type; it must raise, not silently drop the table.
+        """
+        text = """
+            [ccpp-table-properties]
+              name = my_host
+              tpye = host
+
+            [ccpp-arg-table]
+              name = my_host
+              type = host
+            [ im ]
+              standard_name = horizontal_dimension
+              units = count
+              dimensions = ()
+              type = integer
+        """
+        with self.assertRaises(CCPPError) as cm:
+            _parse_text(text)
+        self.assertIn('type', str(cm.exception))
+
+    def test_missing_name_in_table_properties_rejected(self):
+        """A [ccpp-table-properties] block with no ``name`` must raise."""
+        text = """
+            [ccpp-table-properties]
+              type = host
+
+            [ccpp-arg-table]
+              name = my_host
+              type = host
+            [ im ]
+              standard_name = horizontal_dimension
+              units = count
+              dimensions = ()
+              type = integer
+        """
+        with self.assertRaises(CCPPError) as cm:
+            _parse_text(text)
+        self.assertIn('name', str(cm.exception))
+
+    def test_bare_table_properties_header_rejected(self):
+        """A [ccpp-table-properties] header with neither name nor type must
+        raise, listing both missing attributes.
+        """
+        text = """
+            [ccpp-table-properties]
+
+            [ccpp-arg-table]
+              name = my_host
+              type = host
+            [ im ]
+              standard_name = horizontal_dimension
+              units = count
+              dimensions = ()
+              type = integer
+        """
+        with self.assertRaises(CCPPError) as cm:
+            _parse_text(text)
+        m = str(cm.exception)
+        self.assertIn('name', m)
+        self.assertIn('type', m)
+
     def test_finalize_phase_rejected(self):
         """``_finalize`` phase name must raise CCPPError mentioning 'final'."""
         text = """

unit-tests/test_suite_resolver.py

@@ -1883,6 +1883,113 @@ def test_no_flip_on_scalar(self):
         arg = _resolve_one_arg(suite_var, 'run', hd, {}, 'sch', set())
         self.assertFalse(arg.needs_vert_flip)
 
+    def test_backward_expr_uses_conflict_resolved_temp(self):
+        """If a scheme local already occupies the natural temp name, the
+        transform temp is renamed by ``_local_name_conflict``.  The backward
+        (post-call) expression must reference the RENAMED temp, not the raw
+        ``<name>_l`` candidate -- otherwise it silently transforms the
+        scheme's own colliding local instead of the temp.
+        """
+        hd, suite_var = self._build_host_and_scheme(
+            host_units='Pa', scheme_units='hPa', intent='inout')
+        # Pretend the scheme already declared a local named 'temp_l', so the
+        # transform temp must be renamed away from it.
+        used = {'temp_l'}
+        arg = _resolve_one_arg(suite_var, 'run', hd, {}, 'sch', used)
+        self.assertTrue(arg.needs_unit_transform)
+        self.assertNotEqual(arg.temp_name, 'temp_l')   # renamed
+        self.assertTrue(arg.temp_name)
+        # The backward expression references the renamed temp, never 'temp_l'.
+        self.assertIn(arg.temp_name, arg.unit_backward)
+
+    def test_backward_expr_uses_conflict_resolved_temp_when_optional(self):
+        """Same collision guard for the optional+transform path (case 4),
+        where the scheme dummy is passed as a pointer wrapping the temp.  The
+        post-call backward copy must reference the SAME renamed temp that the
+        pointer targets and the declaration declares -- not the raw
+        ``<name>_l`` candidate.
+        """
+        hd, suite_var = self._build_host_and_scheme(
+            host_units='Pa', scheme_units='hPa', intent='inout')
+        suite_var.set_attr('optional', 'True', _ctx())
+        used = {'temp_l'}
+        arg = _resolve_one_arg(suite_var, 'run', hd, {}, 'sch', used)
+        self.assertEqual(arg.transform_case, 4)   # optional AND transform
+        self.assertTrue(arg.is_optional)
+        self.assertTrue(arg.needs_unit_transform)
+        self.assertNotEqual(arg.temp_name, 'temp_l')      # renamed
+        self.assertIn(arg.temp_name, arg.unit_backward)   # backward uses it
+
+    def test_allocatable_host_plus_flip_raises(self):
+        """An allocatable host array whose ``top_at_one`` disagrees with the
+        scheme cannot carry a reverse-stride subscript (allocatable actuals
+        must omit subscripts).  The resolver rejects the combination rather
+        than silently drop the flip and hand over vertically-reversed data.
+        """
+        host_src = '''
+[ccpp-table-properties]
+  name = mod
+  type = host
+[ccpp-arg-table]
+  name = mod
+  type = host
+[ ncols ]
+  standard_name = horizontal_dimension
+  units = count
+  dimensions = ()
+  type = integer
+[ nlev ]
+  standard_name = vertical_layer_dimension
+  units = count
+  dimensions = ()
+  type = integer
+[ gt0 ]
+  standard_name = air_temperature
+  units = K
+  dimensions = (horizontal_dimension, vertical_layer_dimension)
+  type = real
+  kind = kind_phys
+  top_at_one = True
+  allocatable = True
+'''
+        ctrl_src = '''
+[ccpp-table-properties]
+  name = ctrl
+  type = control
+[ccpp-arg-table]
+  name = ctrl
+  type = control
+[ lb ]
+  standard_name = horizontal_loop_begin
+  units = index
+  dimensions = ()
+  type = integer
+[ ub ]
+  standard_name = horizontal_loop_end
+  units = index
+  dimensions = ()
+  type = integer
+'''
+        hd = build_flat_host_dict(_parse(host_src), _parse(ctrl_src), [])
+        from metadata.metadata_table import MetaVar
+        ctx = _ctx()
+        # Scheme wants the same variable with the opposite top_at_one and a
+        # plain (non-allocatable) assumed-shape dummy -> flip required.
+        suite_var = MetaVar('temp', ctx)
+        suite_var.set_attr('standard_name', 'air_temperature', ctx)
+        suite_var.set_attr('units', 'K', ctx)
+        suite_var.set_attr('dimensions',
+                    '(horizontal_dimension, vertical_layer_dimension)', ctx)
+        suite_var.set_attr('type', 'real', ctx)
+        suite_var.set_attr('kind', 'kind_phys', ctx)
+        suite_var.set_attr('intent', 'inout', ctx)
+        with self.assertRaises(CCPPError) as cm:
+            _resolve_one_arg(suite_var, 'run', hd, {}, 'sch', set())
+        msg = str(cm.exception)
+        self.assertIn('air_temperature', msg)
+        self.assertIn('allocatable', msg)
+        self.assertIn('vertical flip', msg)
+
 
 ########################################################################
 # Tests: character kind (len=) validation