chore(phase0): name validation, golden frames, daemon into library tree, TSan daemon lane

Item 8a — ConfigValidator warns on non-identifier variable/event/alarm/
command names ([A-Za-z_][A-Za-z0-9_]*): language bindings expose names as
kwargs/attributes, so 'Chamber Pressure' would be unusable in the planned
Python client. Warning not error — the wire doesn't care. Tested (4 warning
sites + good-name negative).

Item 4 tail — golden frames for S5F1 (Binary ALCD / U4 ALID / ASCII ALTX)
and a composed S6F11 (the production-critical report shape), bytes hand-
computed from E5 encoding rules: external pins on message composition.

Item 7 — equipment_service.hpp moved to include/secsgem/daemon/ (apps/
include-path hack removed) and a TSan daemon lane added locally + in CI.
tools/tsan.supp suppresses races whose accesses sit entirely inside the
UNinstrumented system libgrpc/libgpr/libabsl (epoll wakeups, absl Mutex
GraphCycles bookkeeping); our frames stay fully checked. The lane earned its
keep on first run: it caught a REAL threading-contract violation — a daemon
test reading model stores from the test thread while the io thread serviced
posted writes — fixed to use read_sync, exactly per the documented contract.
Now TSan-clean under halt_on_error=1 in the full production threading shape.

Suites: core 470/3068, daemon Release+TSan 125/125 each.

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

.gitea/workflows/ci.yml

@@ -79,6 +79,7 @@ jobs:
           apt-get install -y --no-install-recommends \
             build-essential cmake ninja-build \
             libasio-dev libyaml-cpp-dev \
+            libprotobuf-dev protobuf-compiler protobuf-compiler-grpc libgrpc++-dev \
             python3 python3-yaml
 
       # Debug + -fsanitize=thread.  Catches data races in the
@@ -98,6 +99,18 @@ jobs:
           TSAN_OPTIONS: halt_on_error=1
         run: build-tsan/secsgem_tests
 
+      # The daemon under TSan: run_async + concurrent gRPC handler threads is
+      # the production threading shape — this is where a strand-contract
+      # violation would actually surface. tools/tsan.supp silences false
+      # positives inside the UNinstrumented system libgrpc only; our frames
+      # stay fully checked.
+      - name: Daemon tests (TSan)
+        env:
+          TSAN_OPTIONS: halt_on_error=1 suppressions=tools/tsan.supp
+        run: |
+          test -x build-tsan/secs_gemd_tests || { echo "secs_gemd_tests not built under TSan"; exit 1; }
+          build-tsan/secs_gemd_tests
+
   tshark-dissector:
     runs-on: ubuntu-latest
     container:

.gitignore

@@ -11,3 +11,4 @@ compile_commands.json
 
 # Local Claude Code agent state (memory, skills, etc.)
 .claude/
+build-tsan-d/

CMakeLists.txt

@@ -269,7 +269,7 @@ if(SECSGEM_DAEMON_BUILT)
     ${PROTO_OUT}/secsgem/v1/equipment.pb.cc
     ${PROTO_OUT}/secsgem/v1/equipment.grpc.pb.cc)
   target_include_directories(secs_gemd_tests PRIVATE
-    ${PROTO_OUT} ${Protobuf_INCLUDE_DIRS} ${CMAKE_SOURCE_DIR}/apps)
+    ${PROTO_OUT} ${Protobuf_INCLUDE_DIRS})
   target_link_libraries(secs_gemd_tests PRIVATE
     secsgem PkgConfig::GRPCPP ${Protobuf_LIBRARIES} doctest::doctest)
   target_compile_definitions(secs_gemd_tests PRIVATE

apps/secs_gemd.cpp

@@ -16,7 +16,7 @@
 #include <memory>
 #include <string>
 
-#include "equipment_service.hpp"
+#include "secsgem/daemon/equipment_service.hpp"
 #include "secsgem/gem/default_handlers.hpp"
 #include "secsgem/gem/runtime.hpp"
 

docs/DAEMON_ROADMAP.md

@@ -48,8 +48,9 @@ host and stays conformant while the tool software restarts/upgrades/crashes.
 - ✅ ~~**Interop harnesses are manual.**~~ `tools/run_interop.sh` runs all nine
   validation steps with one command (verified green); CI lanes added, pending
   first-push verification (Phase 0 item 2).
-- ⬜ **TSan lane doesn't cover the daemon.** `secs_gemd_tests` should also be
+- ✅ ~~**TSan lane doesn't cover the daemon.**~~ Covered locally + in CI
-  built/run under `-DSECSGEM_TSAN=ON` once the control-state mirror lands.
+  with `tools/tsan.supp` (third-party-only suppressions). Caught + fixed a
+  real test-side contract violation on its first run.
 - ⚠️ **macOS bind-mount staleness can break Docker builds mid-edit** (a build
   reading a half-synced source file). Not a product bug; re-run the build.
 
@@ -101,12 +102,10 @@ debts tax every later phase, and the most valuable tests aren't automated.
    localhost, no docker-in-docker). ⬜ Verify the lanes on the first push.
 3. ✅ **Fix `CompleteCommand` proto comment** — it described the rejected
    blocking model; now states the HCACK-4 contract.
-4. 🚧 **Table-driven handler conformance test** — ✅
+4. ✅ **Table-driven handler conformance test** — one ordered scenario
-   `tests/test_handler_conformance.cpp`: one ordered scenario drives 53 of the
+   drives 53 of the 56 handlers through `router.dispatch` (236 assertions).
-   56 handlers through `router.dispatch` in-process (236 assertions), asserting
+   Golden frames: S1F13, S5F1, and a composed S6F11, all hand-computed from
-   paired replies, control-state landings, and the SxF0 abort fallback.
+   E5 rules (external pins, not codec-derived).
-   Message-level golden frames: seeded with a hand-computed (E5-rules, not
-   codec-derived) S1F13 pin — ⬜ extend to S5F1 + composed S6F11 (TODO in file).
 5. ⬜ **Decompose `register_default_handlers` per GEM capability** (it is a
    relocated main(), not a designed component) and replace magic constants
    (SVIDs 1/2 `refresh()`, CEIDs 400/401) with YAML role bindings
@@ -115,14 +114,17 @@ debts tax every later phase, and the most valuable tests aren't automated.
 6. ✅ **Standardize the mutable-read pattern** — `EquipmentRuntime::read_sync`
    (post-to-io + future with deadline; nullopt => UNAVAILABLE at the RPC edge).
    Precedent set by `GetVariables`; every future mutable read copies it.
-7. ⬜ Move `apps/equipment_service.hpp` into the library tree
+7. ✅ `equipment_service.hpp` moved to `include/secsgem/daemon/` (apps/
-   (`include/secsgem/daemon/`) once Phase B grows it; add a TSan-built
+   include-path hack removed). TSan daemon lane added locally + in CI
-   `run_async` + concurrent-RPC daemon test (today's daemon tests only poll()).
+   (`tools/tsan.supp` suppresses UNinstrumented system libgrpc/libabsl
-8. 🚧 Validate names are identifier-safe in `ConfigValidator` (the Python
+   internals only — our frames stay checked). The lane caught a real
-   client's kwargs API depends on it) — ⬜. Generalized format-compliance
+   contract violation on its first run (a test reading the model from the
-   property test (iterates ALL configured variables via gRPC, asserts each
+   test thread under run_async — fixed to read_sync); now TSan-clean with
-   keeps its declared wire format) — ✅, plus an unset-`Value` guard at the
+   halt_on_error=1.
-   RPC edge (was silently writing ASCII "").
+8. ✅ Identifier-safe name validation: `ConfigValidator` warns (not errors)
+   on non-identifier variable/event/alarm/command names — bindings expose
+   names as kwargs/attributes. Format-compliance property test ✅; unset-
+   `Value` guard ✅.
 
 ### Phase A — finish the universal daemon surface (small, unblock vendors)
 1. ✅ `GetVariables` — `from_item` reverse conversion (scalar for 1-element

src/config/validate.cpp

@@ -3,6 +3,7 @@
 #include <yaml-cpp/yaml.h>
 
 #include <algorithm>
+#include <cctype>
 #include <cstdint>
 #include <set>
 #include <string>
@@ -117,6 +118,26 @@ std::optional<std::string> as_nonempty_string(const YAML::Node& n,
   return s;
 }
 
+// API-facing names (variables, events, alarms, commands) should be valid
+// identifiers: language bindings expose them as kwargs/attributes
+// (eq.set(chamber_pressure=...)), where "Chamber Pressure" or "temp-2" can't
+// be written. Warning, not error — the wire doesn't care, only bindings do.
+void warn_if_not_identifier(const std::optional<std::string>& s, Sink& sink,
+                            const std::string& path, const YAML::Node& n) {
+  if (!s) return;
+  bool ok = !s->empty() &&
+            (std::isalpha(static_cast<unsigned char>((*s)[0])) || (*s)[0] == '_');
+  for (std::size_t i = 1; ok && i < s->size(); ++i) {
+    const auto c = static_cast<unsigned char>((*s)[i]);
+    ok = std::isalnum(c) || (*s)[i] == '_';
+  }
+  if (!ok)
+    sink.warn(path, "`" + *s + "` is not identifier-safe " +
+                        "([A-Za-z_][A-Za-z0-9_]*); language bindings expose " +
+                        "names as kwargs/attributes",
+              n);
+}
+
 // Validate one entry that has the same (id, name, units, type, value)
 // shape as SVID/DVID/ECID.  Returns the parsed id on success so the
 // caller can dedupe.
@@ -124,7 +145,8 @@ std::optional<uint32_t> validate_typed_entry(
     const YAML::Node& entry, Sink& sink, const std::string& path) {
   auto id = as_int_in_range<uint32_t>(entry["id"], sink, path + ".id",
                                        0, UINT32_MAX);
-  as_nonempty_string(entry["name"], sink, path + ".name");
+  warn_if_not_identifier(as_nonempty_string(entry["name"], sink, path + ".name"),
+                         sink, path + ".name", entry["name"]);
   if (auto tn = entry["type"]) {
     auto t = tn.as<std::string>();
     if (!valid_secs_type(t)) {
@@ -195,7 +217,9 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) {
         const auto path = "ceids[" + std::to_string(i) + "]";
         auto id = as_int_in_range<uint32_t>(ces[i]["id"], sink, path + ".id",
                                               0, UINT32_MAX);
-        as_nonempty_string(ces[i]["name"], sink, path + ".name");
+        warn_if_not_identifier(
+            as_nonempty_string(ces[i]["name"], sink, path + ".name"),
+            sink, path + ".name", ces[i]["name"]);
         if (id && !ceids.insert(*id).second) {
           sink.error(path + ".id",
                      "duplicate CEID " + std::to_string(*id),
@@ -221,7 +245,9 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) {
         // Optional local key for name-based APIs (not on the wire). If
         // present it must be non-empty.
         if (als[i]["name"])
-          as_nonempty_string(als[i]["name"], sink, path + ".name");
+          warn_if_not_identifier(
+              as_nonempty_string(als[i]["name"], sink, path + ".name"),
+              sink, path + ".name", als[i]["name"]);
         if (id && !alarm_ids.insert(*id).second) {
           sink.error(path + ".id",
                      "duplicate ALID " + std::to_string(*id), als[i]);
@@ -256,6 +282,7 @@ void validate_equipment_block(YAML::Node& root, Sink& sink) {
       for (std::size_t i = 0; i < cmds.size(); ++i) {
         const auto path = "host_commands[" + std::to_string(i) + "]";
         auto name = as_nonempty_string(cmds[i]["name"], sink, path + ".name");
+        warn_if_not_identifier(name, sink, path + ".name", cmds[i]["name"]);
         if (auto ack = cmds[i]["ack"]) {
           auto s = ack.as<std::string>();
           if (!valid_hcack(s))

tests/test_config_validate.cpp

@@ -202,3 +202,32 @@ TEST_CASE("Validate: ships data/equipment.yaml without errors") {
   }
   CHECK(v.error_count() == 0);
 }
+
+TEST_CASE("Validate: non-identifier names warn (bindings expose names as kwargs)") {
+  auto p = scratch_path("idname", "yaml");
+  write(p, R"YAML(
+device:
+  id: 0
+  model_name: "TEST"
+  software_rev: "1.0"
+svids:
+  - {id: 1, name: "Chamber Pressure", type: F4, value: 0.0}
+  - {id: 2, name: GoodName,           type: U4, value: 0}
+ceids:
+  - {id: 10, name: "wafer-complete"}
+alarms:
+  - {id: 1, name: "2bad", text: "T", category: 1}
+host_commands:
+  - {name: "DO IT", ack: Accept}
+)YAML");
+  ConfigValidator v;
+  v.validate_equipment(p.string());
+  CHECK(v.error_count() == 0);   // identifier shape is a WARNING, not an error
+  CHECK(v.warning_count() == 4); // space, hyphen, leading digit, space
+  CHECK(any_match(v.issues(), "svids[0].name"));
+  CHECK(any_match(v.issues(), "ceids[0].name"));
+  CHECK(any_match(v.issues(), "alarms[0].name"));
+  CHECK(any_match(v.issues(), "host_commands[0].name"));
+  CHECK_FALSE(any_match(v.issues(), "svids[1].name"));
+  fs::remove_all(p.parent_path());
+}

tests/test_daemon_service.cpp

@@ -6,7 +6,7 @@
 #include <chrono>
 #include <thread>
 
-#include "equipment_service.hpp"
+#include "secsgem/daemon/equipment_service.hpp"
 #include "secsgem/gem/default_handlers.hpp"
 #include "secsgem/gem/messages.hpp"
 
@@ -220,8 +220,14 @@ TEST_CASE("GetVariables round-trip under run_async (production threading mode)")
     pb::VariableQuery req;
     pb::VariableSnapshot resp;
     REQUIRE(stub->GetVariables(&ctx, req, &resp).ok());
-    const auto expected = rt.model().svids.size() + rt.model().dvids.all().size();
+    // The io thread is live here — model reads must go through read_sync
-    CHECK(resp.values().size() == expected);
+    // (the first violation of that contract was caught by the TSan lane in
+    // exactly this line).
+    auto expected = rt.read_sync([&rt] {
+      return rt.model().svids.size() + rt.model().dvids.all().size();
+    });
+    REQUIRE(expected.has_value());
+    CHECK(resp.values().size() == *expected);
   }
 
   SUBCASE("unknown name is INVALID_ARGUMENT, naming the offender") {

tests/test_handler_conformance.cpp

@@ -172,8 +172,6 @@ TEST_CASE("conformance sweep: every default handler answers with the paired repl
 //   L[2]          -> 0x01 0x02
 //   A "HOST"      -> 0x41 0x04 'H' 'O' 'S' 'T'
 //   A "1.0"       -> 0x41 0x03 '1' '.' '0'
-// TODO(tests): extend with golden frames for S5F1 and a composed S6F11 once
-// their ALID/CEID item formats are pinned down from the catalog.
 TEST_CASE("golden frame: S1F13 body encodes to the hand-computed E5 bytes") {
   auto msg = gem::s1f13_establish_comms("HOST", "1.0");
   REQUIRE(msg.body.has_value());
@@ -187,3 +185,40 @@ TEST_CASE("golden frame: S1F13 body encodes to the hand-computed E5 bytes") {
   CHECK(msg.function == 13);
   CHECK(msg.reply_expected);
 }
+
+// S5F1 = L[3]{ Binary ALCD, U4 ALID, ASCII ALTX }. Format bytes per E5:
+// Binary(0o10=8)->0x21, U4(0o54=44)->0xB1, ASCII(0o20=16)->0x41 (1 len byte).
+TEST_CASE("golden frame: S5F1 alarm report encodes to the hand-computed E5 bytes") {
+  auto msg = gem::s5f1_alarm_report(0x84, 1, "Chiller Temp High");
+  REQUIRE(msg.body.has_value());
+  std::vector<uint8_t> expected = {
+      0x01, 0x03,                                  // L/3
+      0x21, 0x01, 0x84,                            // B  ALCD = set|category4
+      0xB1, 0x04, 0x00, 0x00, 0x00, 0x01,          // U4 ALID = 1
+      0x41, 0x11,                                  // A/17
+  };
+  for (char c : std::string("Chiller Temp High")) expected.push_back(c);
+  CHECK(s2::encode(*msg.body) == expected);
+  CHECK(msg.stream == 5);
+  CHECK(msg.function == 1);
+}
+
+// Composed S6F11 = L[3]{ U4 DATAID, U4 CEID, L reports[ L[2]{U4 RPTID,
+// L values} ] } — the production-critical event-report shape, pinned for one
+// report (RPTID 1) carrying one ASCII value.
+TEST_CASE("golden frame: composed S6F11 encodes to the hand-computed E5 bytes") {
+  std::vector<gem::ReportData> reports{{1, {s2::Item::ascii("x")}}};
+  auto msg = gem::s6f11_event_report(0, 300, reports);
+  REQUIRE(msg.body.has_value());
+  const std::vector<uint8_t> expected = {
+      0x01, 0x03,                                  // L/3
+      0xB1, 0x04, 0x00, 0x00, 0x00, 0x00,          // U4 DATAID = 0
+      0xB1, 0x04, 0x00, 0x00, 0x01, 0x2C,          // U4 CEID = 300
+      0x01, 0x01,                                  //   L/1 reports
+      0x01, 0x02,                                  //     L/2 report
+      0xB1, 0x04, 0x00, 0x00, 0x00, 0x01,          //       U4 RPTID = 1
+      0x01, 0x01,                                  //       L/1 values
+      0x41, 0x01, 'x',                             //         A "x"
+  };
+  CHECK(s2::encode(*msg.body) == expected);
+}

tools/tsan.supp

@@ -0,0 +1,20 @@
+# ThreadSanitizer suppressions — third-party noise only.
+#
+# The system libgrpc/libprotobuf are NOT built with TSan instrumentation, so
+# TSan cannot observe their internal synchronization and reports false
+# positives entirely inside the library (e.g. Epoll1Poller::DoEpollWait's
+# event-engine wakeup path). Suppressing by library keeps every frame of OUR
+# code fully checked: a real race in secsgem/daemon code still has our frames
+# in the stack and is NOT suppressed by these rules.
+#
+# Do not add suppressions for secsgem code here. Fix the race instead.
+#
+# libabsl_*: gRPC's absl Mutex keeps deadlock-detection bookkeeping
+# (GraphCycles) whose synchronization TSan cannot see in the uninstrumented
+# system build — every report's racing accesses sit fully inside
+# libabsl_graphcycles_internal/libabsl_malloc_internal frames.
+race:libgrpc.so
+race:libgrpc++.so
+race:libgpr.so
+race:libabsl_
+called_from_lib:libgrpc.so