Lint C++ header contracts and table-driven native test cases

AGENTS.md

@@ -215,8 +215,10 @@ See [pokemon_sv/PR_WORKFLOW.md](pokemon_sv/PR_WORKFLOW.md). In short:
   - module dependencies point one way
 - **Tests:** native C++ unit tests registered in the game's `*_Tests.cpp`
   (e.g. `PokemonSV_Tests.cpp`).
-  - Data-driven, with designated initializers so every value has a field name,
-    and no if/else-if chains.
+  - Data-driven: separate input and expectation structs, a test-case struct with
+    exactly `input` and `expectation` fields, and a named table of designated
+    case initializers. A UnitTest receives one case; a range-for registers the
+    table rows. Code Health checks new/changed test classes. No if/else-if chains.
   - Failure messages say what was expected and what was read.
   - No Python or source-extraction tests.
 - **Seams:** to test controller logic, inject the game steps through a small
@@ -268,15 +270,22 @@ so keep it where it clearly pays off. `std::string_view` is the exception (see S
   Files are `PokemonFRLG_Navigation.h/.cpp`. Not Google's `kName`, `name_`, `PascalCase()`.
 - **Formatting:** 4 spaces, CRLF, `if (x){`, `}else{`, `namespace X{`, `class A : public B{`,
   always braced multi-line bodies. No 80-column limit. Wrap long parameter lists the way
-  nearby code does. Close namespaces with a bare `}`.
+  nearby code does. Close named namespaces with `} // namespace Name` and anonymous
+  namespaces with `} // namespace`; the fork Code Health gate checks changed closes.
 - **`virtual ... override`** on overrides (Google says `override` alone).
 - **Guards and includes:** `PokemonAutomation_<Name>_H`. Include order per
   IncludeOrder.md with the file's own header **last** (Google puts it first).
 - **Exceptions are used** (Google bans them). Game failure:
   `OperationFailedExceptionWithScreenshot::fire()` / `throw_and_log<OperationFailedException>`.
   Config problem: `UserSetupError`. Programmer bug (Google's `CHECK`): `InternalProgramError`.
   Let `ProgramCancelledException` and `ProgramFinishedException` propagate.
-- **Forward declarations:** stricter than Google. None in our code; include the header.
+- **Forward declarations:** include real headers instead of forward-declaring types.
+  Function templates in headers declare their public API before internal helper
+  namespaces; internal helper templates are defined in place. Definitions stay
+  in the same header; this is not permission to forward-declare external types.
+- **Header documentation:** comment every namespace-scope symbol, including
+  types, aliases, constants and internal helpers. A function definition may reuse
+  its documented declaration in the same header. Code Health checks changed symbols.
 - **Unsigned:** counts and indexes are `size_t` / `uint8_t` (Google prefers `int`).
   Match that, and see TotW #227 below.
 - **Callbacks:** APIs such as `run_until()` take `std::function<...>&&`. Match the API

devtools/code_health/README.md

@@ -196,3 +196,53 @@ need appropriate live run evidence.
 `metrics.py` and `policy.py` accept source/metrics as values and know nothing
 about this repository's Git layout. `git_files.py` and `__main__.py` are the
 integration layer; extract a separate package only when another consumer needs it.
+
+## Header and native test contracts
+
+The fork's structural checks in `style.py` run in both the normal CLI and
+pre-push/CI gate. They share the existing parsing pass and comment/literal-aware
+scope lexer, not raw line
+matches or application source-extraction tests. Only changed namespace closes
+and declarations are enforced; untouched legacy declarations are grandfathered.
+
+- `namespace-close`: named namespaces end in `} // namespace Name`, anonymous
+  ones in `} // namespace`. The name must match, including C++ nested namespace syntax.
+- `template-api`: public namespace-scope function templates in headers declare their
+  interface before their definition. Inline class members and class templates
+  do not need redundant declarations. Keep default arguments on the declaration.
+- `template-api-order`: public template declarations precede helper namespaces
+  named `*Internal`, `internal`, or `detail`.
+- `template-api-internal`: internal helper templates are defined in place, with
+  no separate forward declaration. A header containing only internal helpers
+  needs no invented public API.
+- `header-comment`: document each added/changed namespace-scope header symbol,
+  including helper declarations, types, aliases, and constants. An immediately
+  adjacent `//` or block/Doxygen comment counts. A definition can reuse its own
+  documented declaration. Class members and namespaces themselves are excluded.
+- `test-case`: new/changed native `*_Tests.cpp` UnitTest classes take one aggregate case.
+  That case has exactly `input` and `expectation` fields, each a separate local
+  struct. A named C-array, `std::array`, or `std::vector` table contains designated
+  `.input` / `.expectation` rows and is consumed by a range-for for registration.
+  Include a descriptive name in the input and compare expected and observed values.
+
+```cpp
+struct ValueInput{ std::string name; int value; };
+struct ValueExpectation{ int result; };
+struct ValueTestCase{ ValueInput input; ValueExpectation expectation; };
+
+const ValueTestCase CASES[] = {
+    {
+        .input = {.name = "increment", .value = 1},
+        .expectation = {.result = 2},
+    },
+};
+// Test_Value receives ValueTestCase and compares actual results with expectation.
+// Register with: for (const auto& scenario : CASES){ database.add<Test_Value>(scenario); }
+```
+
+The native shape follows PokemonAutomation/Arduino-Source#1846. The lexical
+checker validates local structure; it cannot resolve included aliases, macro
+generated declarations or C++ overloads like a compiler. Shared fixture types
+should remain beside the tests or need a deliberate parser extension, not an
+unexplained bypass. Header parameter spans, game movement policies and comment
+quality still require design review. These rules do not force a default policy.

devtools/code_health/style.py

@@ -486,6 +486,222 @@ def comment_checks(path, raw_lines, statements_, added):
     return warnings
 
 
+# ---- Header and native test structure -----------------------------------------------
+
+def scopes(parsed):
+    """Yield each statement and its enclosing (kind, name) brace frames."""
+    stack = []
+    for statement in parsed:
+        yield statement, tuple(stack)
+        if statement.end == '{':
+            kind = brace_kind(statement.text)
+            if re.fullmatch(r'extern\s*"\s*"', statement.text):
+                kind = 'linkage'
+            match = re.fullmatch(r'(?:inline\s+)?namespace\s*([\w:]*)', statement.text)
+            if kind == 'class':
+                match = re.search(r'\b(?:class|struct|union)\s+(\w+)', statement.text)
+            stack.append((kind, match.group(1) if match else ''))
+        elif statement.end == '}' and stack:
+            stack.pop()
+
+
+def namespace_closes(path, lines, parsed, changed):
+    errors = []
+    for statement, enclosing in scopes(parsed):
+        if statement.end != '}' or statement.line not in changed or not enclosing:
+            continue
+        kind, name = enclosing[-1]
+        if kind != 'namespace':
+            continue
+        expected = '// namespace' + (' ' + name if name else '')
+        if not re.search(r'}\s*' + re.escape(expected) + r'\s*$', lines[statement.line - 1]):
+            errors.append(Finding(path, statement.line,
+                                  f'namespace-close: close this namespace with {expected}'))
+    return errors
+
+
+def parameter_type(param):
+    """Ignore ordinary parameter names/defaults when matching declarations."""
+    declared = default_split(param)[0]
+    last = re.search(r'\b([A-Za-z_]\w*)$', declared)
+    if not last:
+        return declared
+    prefix = declared[:last.start()].rstrip()
+    qualifiers = {'const', 'volatile', 'unsigned', 'signed', 'long', 'short'}
+    if not prefix or prefix.endswith('::') or set(prefix.split()) <= qualifiers:
+        return declared
+    return prefix
+
+
+def function_key(statement, enclosing):
+    sig = signature(statement)
+    if sig is None or not statement.namespace_scope:
+        return None
+    return (enclosing, sig.name, tuple(parameter_type(param) for param in sig.params))
+
+
+def template_api(path, parsed, changed):
+    """Template free functions declare their interface before implementation."""
+    errors, declared = [], set()
+    for statement, enclosing in scopes(parsed):
+        key = function_key(statement, enclosing)
+        if key is None or not statement.text.startswith('template'):
+            continue
+        helper = any(internal(name) for _, name in enclosing)
+        if helper:
+            if statement.end == ';' and statement.line in changed:
+                errors.append(Finding(path, statement.line,
+                    'template-api-internal: define internal function templates in place'))
+            continue
+        if statement.end == ';':
+            declared.add(key)
+        elif statement.end == '{' and key not in declared and statement.line in changed:
+            errors.append(Finding(path, statement.line,
+                                  'template-api: declare the function template before defining it'))
+    errors += template_order(path, parsed, changed)
+    return errors
+
+
+def internal(name):
+    return name.endswith('Internal') or name in ('detail', 'internal')
+
+
+def template_order(path, parsed, changed):
+    declarations = [(enclosing, statement.line) for statement, enclosing in scopes(parsed)
+                    if statement.end == ';' and statement.text.startswith('template')
+                    and function_key(statement, enclosing)
+                    and not any(internal(name) for _, name in enclosing)]
+    errors = []
+    for statement, enclosing in scopes(parsed):
+        match = re.fullmatch(r'namespace\s+(\w+)', statement.text)
+        if statement.end != '{' or not match or not internal(match.group(1)):
+            continue
+        if any(parent == enclosing and line > statement.line
+               and (statement.line in changed or line in changed)
+               for parent, line in declarations):
+            errors.append(Finding(path, statement.line,
+                                  'template-api-order: declare public templates before internal helpers'))
+    return errors
+
+
+def documented(lines, line):
+    """Accept adjacent // prose or a closed block comment, including Doxygen."""
+    if line < 2:
+        return False
+    above = lines[line - 2].strip()
+    if above.startswith('//'):
+        return bool(above.lstrip('/').strip())
+    if not above.endswith('*/'):
+        return False
+    for earlier in reversed(lines[:line - 1]):
+        if '/*' in earlier:
+            return True
+        if not earlier.strip().startswith(('*', '//')):
+            return False
+    return False
+
+
+def header_comments(path, lines, parsed, changed):
+    errors, documented_functions = [], set()
+    for statement, enclosing in scopes(parsed):
+        text = statement.text
+        if not statement.namespace_scope or statement.end == '}':
+            continue
+        if not text or re.match(r'(?:inline\s+)?namespace\b|(?:static_assert|extern\s*")', text):
+            continue
+        key = function_key(statement, enclosing)
+        has_comment = documented(lines, statement.line)
+        if has_comment and key:
+            documented_functions.add(key)
+        if statement.line in changed and not has_comment and key not in documented_functions:
+            errors.append(Finding(path, statement.line,
+                                  'header-comment: document each namespace-scope header symbol'))
+    return errors
+
+
+def structures(parsed):
+    """Local aggregate definitions and their directly declared fields."""
+    found = {}
+    for statement, enclosing in scopes(parsed):
+        match = re.match(r'struct\s+(\w+)\b', statement.text)
+        if match and statement.end == '{':
+            found[match.group(1)] = {}
+        elif enclosing and enclosing[-1][0] == 'class' and statement.end == ';':
+            fields = found.get(enclosing[-1][1])
+            field = re.fullmatch(r'([\w:<> ,]+?)\s+(\w+)\s*(?:=.*)?', statement.text)
+            if fields is not None and field:
+                fields[field.group(2)] = field.group(1)
+    return found
+
+
+def case_type(test_name, parsed):
+    """The single aggregate parameter of this UnitTest constructor, if present."""
+    for statement, enclosing in scopes(parsed):
+        if not enclosing or enclosing[-1] != ('class', test_name):
+            continue
+        constructor = re.match(r'(?:(?:public|protected|private)\s*:\s*)?(?:explicit\s+)?' + re.escape(test_name) + r'\s*\(', statement.text)
+        if constructor:
+            end = matching(statement.text, constructor.end() - 1)
+            params = split_top_level(statement.text[constructor.end():end])
+            if len(params) == 1:
+                param = re.fullmatch(r'(?:const\s+)?(\w+)\s*(?:&&|&)?\s+\w+', params[0])
+                return param.group(1) if param else None
+    return None
+
+
+def designated_case(row):
+    row = row.strip()
+    if not row.startswith('{') or not row.endswith('}'):
+        return False
+    fields = [field for field in split_top_level(row[1:-1]) if field]
+    names = [re.match(r'\.(input|expectation)\s*=', field) for field in fields]
+    return len(names) == 2 and all(names) and {name.group(1) for name in names} == {'input', 'expectation'}
+
+
+def case_table(case, code):
+    """A named aggregate table consumed by a range-for, not ad hoc registrations."""
+    element = re.escape(case)
+    declaration = re.compile(
+        r'(?:const\s+|constexpr\s+|static\s+|inline\s+)*'
+        r'(?:' + element + r'\s+(\w+)\s*\[\s*\]|'
+        r'std::(?:vector|array)\s*<\s*' + element + r'(?:\s*,[^>]+)?\s*>\s+(\w+))\s*(?:=|{)')
+    for table in declaration.finditer(code):
+        name = table.group(1) or table.group(2)
+        opening = code.find('{', table.end() - 1)
+        closing = matching(code, opening, '{', '}') if opening >= 0 else -1
+        if closing < 0:
+            continue
+        rows = [row for row in split_top_level(code[opening + 1:closing]) if row]
+        designated = all(designated_case(row) for row in rows)
+        if rows and designated and re.search(r'\bfor\s*\([^;]*?:\s*' + re.escape(name) + r'\s*\)', code):
+            return True
+    return False
+
+
+def native_cases(path, code, parsed, changed):
+    """New or changed native UnitTest classes use one structured, table-driven scenario."""
+    definitions, errors = structures(parsed), []
+    for statement, _ in scopes(parsed):
+        test = re.match(r'(?:class|struct)\s+(\w+)\s*:\s*public\s+(?:[\w:]+::)?UnitTest\b', statement.text)
+        if not test or statement.end != '{':
+            continue
+        edited = statement.line in changed or any(
+            nested.line in changed and ('class', test.group(1)) in enclosing
+            for nested, enclosing in scopes(parsed))
+        if not edited:
+            continue
+        case = case_type(test.group(1), parsed)
+        fields = definitions.get(case, {})
+        valid_fields = set(fields) == {'input', 'expectation'} and all(
+            field in definitions for field in fields.values())
+        if not case or not valid_fields or not case_table(case, code):
+            errors.append(Finding(path, statement.line,
+                'test-case: construct each UnitTest from one case struct with input and expectation '
+                'struct fields; register a named table of designated cases in a range-for'))
+    return errors
+
+
+
 # ---- Entry point -------------------------------------------------------------------
 
 def check(path, source, previous_source):
@@ -507,5 +723,11 @@ def check(path, source, previous_source):
     more_errors, warnings = statement_checks(path, statements_, added,
                                              bool_signatures(previous_statements),
                                              previous_source is None)
+    errors += namespace_closes(path, raw_lines, statements_, added)
+    if path.endswith('_Tests.cpp'):
+        errors += native_cases(path, '\n'.join(code), statements_, added)
+    if path.endswith(HEADERS):
+        errors += template_api(path, statements_, added)
+        errors += header_comments(path, raw_lines, statements_, added)
     warnings += comment_checks(path, raw_lines, statements_, added)
     return sorted(errors + more_errors, key=lambda f: f.line), sorted(warnings, key=lambda f: f.line)

devtools/code_health/tests/test_cli.py

@@ -138,7 +138,7 @@ def test_class_shape_applies_to_new_classes_only(self):
         (self.root / 'Common/old.h').write_text(interface)
         self.commit()
         with (self.root / 'Common/old.h').open('a') as file:
-            file.write('int helper();\n')
+            file.write('// An unrelated documented declaration.\nint helper();\n')
         self.git('add', '.')
         self.assertEqual(0, self.cli().returncode)
         (self.root / 'Common/new.h').write_text(interface.replace('Listener', 'Fresh'))
@@ -212,6 +212,23 @@ def test_allow_tag_bypasses_a_limit_visibly(self):
         self.assertEqual(1, result.returncode, result.stderr)
         self.assertIn('justification', result.stderr)
 
+    def test_structural_header_findings_reach_cli_and_annotations(self):
+        self.source(3)
+        self.commit()
+        header = self.root / 'Common/policy.h'
+        header.write_text('namespace Policy{\ntemplate <typename T> T run(T value){ return value; }\n}\n')
+        self.git('add', '.')
+        red = self.cli('--github')
+        self.assertEqual(1, red.returncode)
+        for rule in ('namespace-close', 'template-api', 'header-comment'):
+            self.assertIn(rule, red.stderr)
+        self.assertIn('::error file=Common/policy.h', red.stdout)
+        header.write_text('namespace Policy{\n// Return the value.\n'
+                          'template <typename T> T run(T value);\n'
+                          'template <typename T> T run(T value){ return value; }\n'
+                          '} // namespace Policy\n')
+        self.assertEqual(0, self.cli().returncode)
+
 
 if __name__ == '__main__':
     unittest.main()