From 39ce5057bf2ccd33246ff78934775c1ed86d41b4 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 18:01:08 +0200 Subject: [PATCH 01/13] docs: design for unordered sections (#513) SectionSet -- N independently editable action drafts, each gated on its own ActionValidator::ready, no sequencing. The sibling to FlowSession for the screen shape 779bd8aa's removal left without a target. Decisions the design records, and why each was chosen over the alternative: - Full parity with FlowSession (prefill binds, result capture, schema JSON) rather than the minimal draft-and-fire mechanism #513 literally describes. - Prefill is reactive -- a bound field fills when its source fires, whenever that is -- rather than resolved lazily at read time (which pushes the work back onto the consumer, the thing #513 objects to) or requiring an already-fired source (which reintroduces sequencing through the back door). - **A prefill write never fires its section.** This is the one constraint chosen rather than requested: without it, A completing could cascade-fire B, and two mutually bound sections would ping-pong. The cost is accepted and written down -- a section made ready purely by prefill waits for one user edit. - No latch: a ready section re-fires on every set<>, as FlowSession does. - Parallel Section/SectionGroup declaration types rather than reusing WizardStep, so a consumer declaring an unordered screen does not write "WizardStep". The shared machinery moves to forms/detail/session_common.hpp; flows.hpp keeps every name it exports today. The testing section names nine cases and, for each, what it fails without -- case 5 asserts both halves of the prefill rule, since a test that only checked the field filled would pass under a cascading implementation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- .../2026-09-10-unordered-sections-design.md | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-10-unordered-sections-design.md diff --git a/docs/superpowers/specs/2026-09-10-unordered-sections-design.md b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md new file mode 100644 index 00000000..d8a0d19a --- /dev/null +++ b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md @@ -0,0 +1,196 @@ +# Unordered sections — design + +`morph::forms::SectionSet` gives a screen N independently +editable action drafts, each gated on its own `ActionValidator::ready` and +each dispatched through the handler's ordinary `execute()`. No sequence, no +current step, no advance/back. + +Closes [morph#513](https://github.com/LASTRADA-Software/morph/issues/513). + +## Contents + +- [The gap](#the-gap) +- [Headers](#headers) +- [Declaration layer](#declaration-layer) +- [SectionSet](#sectionset) +- [Reactive prefill](#reactive-prefill) +- [Concurrency and lifetime](#concurrency-and-lifetime) +- [Schema document](#schema-document) +- [Testing](#testing) +- [Out of scope](#out-of-scope) + +## The gap + +`779bd8aa` removed the handler-side reactive draft (`BridgeHandler::set<&A::field>`, +`reset`, the action-keyed `subscribe`) and named +`morph::flows::FlowSession` as its replacement. `FlowSession` serves a wizard: +one active step at a time, enforced at `flows.hpp:293`, which throws +`std::logic_error` for a field belonging to any other step. + +That leaves the screen shape the removed mechanism also served — N sections a +user edits in any order, each with its own draft and its own readiness gate — +with no in-framework target. A consumer either reimplements draft accumulation +locally or restructures the screen into a wizard it is not. + +`SectionSet` is the sibling for that shape. + +## Headers + +| File | Holds | +| --- | --- | +| `forms/detail/session_common.hpp` *(new)* | `Bind`, `forEachTupleElement`, `forPackElement`, `AllDistinct`, prefill-path resolution, schema-emission helper | +| `forms/flows.hpp` | `Wizard`, `WizardStep`, `FlowSession` — unchanged behaviour | +| `forms/sections.hpp` *(new)* | `Section`, `SectionGroup`, `SectionGroupTraits`, `BRIDGE_REGISTER_SECTION_GROUP`, `sectionGroupSchemaJson`, `SectionSet` | + +The shared header is an extraction, not a rewrite: `flows.hpp` keeps every +public name it exports today and gains an include. Splitting it this way keeps +`flows.hpp` at its present size instead of doubling it, and gives `Bind` — which +both session types genuinely share — one home rather than two. + +## Declaration layer + +`Section` mirrors `WizardStep` structurally and is named for its own domain: + +```cpp +template +struct Section { + using action = Action; + using binds = std::tuple; + [[nodiscard]] static constexpr std::string_view title() noexcept; +}; + +template +struct SectionGroup { + using sections = std::tuple; + [[nodiscard]] static constexpr std::string_view title() noexcept; +}; +``` + +`SectionGroupTraits` maps a group type to its string type-id, specialised via +`BRIDGE_REGISTER_SECTION_GROUP(G, "name")`. It exists for the same reason +`WizardTraits` does: `sectionGroupSchemaJson()` needs a stable name for the +group in the emitted document, and deriving one from the C++ type would tie the +wire format to a mangled name. + +The two trait structs are the only duplication against `flows.hpp`; everything +substantive lives in the shared header. Reusing `WizardStep` for an unordered +screen was rejected: a consumer declaring sections would write `WizardStep`, +which is misleading in exactly the place morph#513 says the model is wrong. + +## SectionSet + +```cpp +template +class SectionSet { + static_assert(sizeof...(Sections) > 0); + static_assert(AllDistinct::value); + +public: + explicit SectionSet(BridgeHandler& handler, + std::function onError = nullptr); + ~SectionSet(); // requestStop() first + + template void set(ValueType value); + template void reset(); + template [[nodiscard]] A draft() const; +}; +``` + +`set<>` keeps `FlowSession`'s compile-time check that the field's action belongs +to the set and drops the runtime current-step `throw`. It takes `_mtx`, writes +the field, copies the draft out, releases the lock, and dispatches through +`handler.execute()` when `ActionValidator::ready(draft)` holds — the same +sequence `FlowSession::set<>` runs, minus the step check. + +**A ready section re-fires on every subsequent `set<>`.** There is no latch and +no fire-once-per-ready-transition rule. This matches `FlowSession`, which +documents that a caller wanting one request per pause debounces on its own side; +a section that silently stopped dispatching after its first success would be a +surprising rule to carry. + +`reset()` returns that section's draft to `A{}` and re-applies any prefill +already resolved for it. It touches no other section. + +`draft()` returns a snapshot for a renderer to display. It is a copy taken +under the lock, not a reference into live state. + +## Reactive prefill + +A section declaring `Bind<&B::field, "A.result">` is filled when `A` produces +its result, whenever that happens. `FlowSession` never faced this question — +its ordering guarantees the source has already fired. Without ordering, the +alternatives were to resolve lazily at read time (which pushes the work back +onto the consumer, the thing morph#513 objects to) or to require a fired source +(which reintroduces sequencing through the back door). + +On a successful result the fields are recorded into `_resolvedValues` under +`"."`, then every *other* section with a `Bind` naming one +of those paths has its draft field written. + +**A prefill write never fires its section.** Only `set<>` does. + +This is the one constraint chosen rather than requested, and it is load-bearing: +without it, `A` completing could cascade-fire `B`, and two sections bound to +each other would ping-pong. The cost is visible and accepted — a section made +ready purely by prefill waits for one user edit before dispatching. + +## Concurrency and lifetime + +One `_mtx` guards `_drafts` and `_resolvedValues`. It is never held across +`handler.execute()`, and the draft is copied out before dispatch, matching +`FlowSession`. + +One `CallbackScope` gates every installed continuation, and the destructor calls +`requestStop()` explicitly as its first statement rather than leaving it to the +member's own destruction — the same reasoning `FlowSession`'s destructor +records, so a body that later grows a pumping call cannot deliver into a +half-dead session. + +The guarantee has the same boundary as `FlowSession`'s and the same caveat +applies verbatim: destroying the session on the thread its continuations are +delivered on makes check-then-run atomic; destroying it from another thread is +advisory, and that caller owns its own synchronisation. See +`docs/spec/core/callback_scope.md`, "Boundary of the guarantee". + +## Schema document + +`sectionGroupSchemaJson()` emits the group title and one entry per section +carrying its title, action type-id and declared binds. It mirrors +`wizardSchemaJson`'s shape with `s-` keys instead of `w-`, and deliberately +carries no index or order field: a renderer laying out sections chooses its own +arrangement, and emitting a position would imply a sequence the type does not +have. + +## Testing + +`tests/test_sections.cpp`. Every case is written to fail without the feature — +a suite that would pass either way measures nothing. + +| # | Case | Fails without | +| --- | --- | --- | +| 1 | Edit section B, then A; both fire | The feature entirely — this is the shape `FlowSession` throws on | +| 2 | A not-ready draft does not dispatch; a ready one does | The readiness gate | +| 3 | Editing an already-fired section fires again | The no-latch rule | +| 4 | `reset()` clears A, leaves B's draft intact | Per-section isolation | +| 5 | A fires → B's bound field fills, **and B does not fire** | The prefill-never-fires rule, in both directions | +| 6 | `onError` runs on dispatch failure; default path logs | Error routing | +| 7 | Destroying the set with a dispatch in flight delivers nothing | The `CallbackScope` gate | +| 8 | Schema carries each section's title and action id | Schema emission | +| 9 | Duplicate action types, and a field outside the set, are rejected | The two `static_assert`s | + +Case 5 is the one worth writing carefully: it must assert both halves, because +a test that only checks the field filled would pass under a cascading +implementation. + +Case 9 is compile-time. It is covered by a documented negative example rather +than a runtime assertion, since a `static_assert` that fires cannot also be +linked into the suite. + +## Out of scope + +- Changing `FlowSession`. Its single-active-step constraint is correct for a + wizard and stays. +- In-flight coalescing. It was removed with the reactive draft in `779bd8aa` + and is not reintroduced here; `FlowSession` does without it too. +- A QML renderer for section groups. The schema document is emitted; consuming + it is a separate piece of work. From 5d406495009453d8da793ce75eaeed37a3d7acca Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:20:10 +0200 Subject: [PATCH 02/13] =?UTF-8?q?docs:=20correct=20the=20prefill=20design?= =?UTF-8?q?=20=E2=80=94=20FlowSession=20does=20not=20write=20drafts=20(#51?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design I wrote and got approved said a bound field is written into its section's draft when the source fires, and called that parity with `FlowSession`. It is not. Found while gathering the concrete code for the implementation plan, before any of it was built. `binds` are consumed in exactly one place in flows.hpp (:166-171): emitted into the schema document under a `prefill` node, for a renderer. The runtime side is `resolved(path)` (:419), which hands the caller a JSON string. `FlowSession` never writes a prefill value into a draft, and no such mechanism exists anywhere in the tree -- it would need JSON-to-typed-field deserialization keyed by field name. So the section is rewritten to what parity actually is: binds are a declaration, `sectionGroupSchemaJson` emits them, results are captured into `_resolvedValues`, and `resolved(path)` exposes them. The renderer decides whether to overwrite a field the user has already touched -- a judgement the framework has no basis for making. Two consequences recorded rather than quietly dropped: - The "a prefill write never fires its section" rule is gone. It existed only to constrain a write the framework will not be doing, so it was a constraint on nothing. - Test case 5 changes from "B's field fills and B does not fire" to asserting `resolved()` on both a captured path and an uncaptured one. The rejected mechanism is written into Out of scope with its cost, so the next reader does not re-propose it without knowing what it needs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- .../2026-09-10-unordered-sections-design.md | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/docs/superpowers/specs/2026-09-10-unordered-sections-design.md b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md index d8a0d19a..a7d7c509 100644 --- a/docs/superpowers/specs/2026-09-10-unordered-sections-design.md +++ b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md @@ -13,7 +13,7 @@ Closes [morph#513](https://github.com/LASTRADA-Software/morph/issues/513). - [Headers](#headers) - [Declaration layer](#declaration-layer) - [SectionSet](#sectionset) -- [Reactive prefill](#reactive-prefill) +- [Prefill](#prefill) - [Concurrency and lifetime](#concurrency-and-lifetime) - [Schema document](#schema-document) - [Testing](#testing) @@ -93,6 +93,7 @@ public: template void set(ValueType value); template void reset(); template [[nodiscard]] A draft() const; + [[nodiscard]] std::optional resolved(std::string_view path) const; }; ``` @@ -114,25 +115,34 @@ already resolved for it. It touches no other section. `draft()` returns a snapshot for a renderer to display. It is a copy taken under the lock, not a reference into live state. -## Reactive prefill +## Prefill -A section declaring `Bind<&B::field, "A.result">` is filled when `A` produces -its result, whenever that happens. `FlowSession` never faced this question — -its ordering guarantees the source has already fired. Without ordering, the -alternatives were to resolve lazily at read time (which pushes the work back -onto the consumer, the thing morph#513 objects to) or to require a fired source -(which reintroduces sequencing through the back door). +A section declaring `Bind<&B::field, "A.result">` publishes a *declaration*, not +a write. This matches `FlowSession` exactly, and the parity is worth stating +because it is easy to assume otherwise: `binds` are consumed in exactly one +place in `flows.hpp` — emitted into the schema document under a `prefill` node +for a renderer. `FlowSession` never writes a prefill value into a draft. -On a successful result the fields are recorded into `_resolvedValues` under -`"."`, then every *other* section with a `Bind` naming one -of those paths has its draft field written. +`SectionSet` does the same two things: -**A prefill write never fires its section.** Only `set<>` does. +- `sectionGroupSchemaJson()` emits each section's binds under `prefill`, as + `{ "": "." }`. +- On a successful result, the submitted draft's fields and then the result's + fields are recorded into `_resolvedValues` under `"."`, + result fields winning on a name collision — the same order `FlowSession` + records them in. +- `resolved(path)` returns a captured field's JSON-encoded value, or + `std::nullopt` if that path was never captured. -This is the one constraint chosen rather than requested, and it is load-bearing: -without it, `A` completing could cascade-fire `B`, and two sections bound to -each other would ping-pong. The cost is visible and accepted — a section made -ready purely by prefill waits for one user edit before dispatching. +The renderer decides what to do with a resolved value: it is the component that +knows whether a field the user has already edited should be overwritten, and +the framework has no basis for that judgement. + +**No ordering is implied.** In a wizard a bind's source always precedes its +target, so `resolved` is populated by the time a step is entered. Here a bind +may name a section that has not fired, and `resolved` returns `std::nullopt` +for it — which is the same answer `FlowSession` gives for a path that was never +captured, so the accessor's contract is unchanged. ## Concurrency and lifetime @@ -172,15 +182,15 @@ a suite that would pass either way measures nothing. | 2 | A not-ready draft does not dispatch; a ready one does | The readiness gate | | 3 | Editing an already-fired section fires again | The no-latch rule | | 4 | `reset()` clears A, leaves B's draft intact | Per-section isolation | -| 5 | A fires → B's bound field fills, **and B does not fire** | The prefill-never-fires rule, in both directions | +| 5 | A fires → `resolved("A.field")` returns its value; a path whose section has not fired returns `nullopt` | Result capture and the resolved-values map | | 6 | `onError` runs on dispatch failure; default path logs | Error routing | | 7 | Destroying the set with a dispatch in flight delivers nothing | The `CallbackScope` gate | | 8 | Schema carries each section's title and action id | Schema emission | | 9 | Duplicate action types, and a field outside the set, are rejected | The two `static_assert`s | -Case 5 is the one worth writing carefully: it must assert both halves, because -a test that only checks the field filled would pass under a cascading -implementation. +Case 5 asserts both halves — a captured path and an uncaptured one — because a +test that only checked the captured case would pass against an implementation +that returned a value for everything. Case 9 is compile-time. It is covered by a documented negative example rather than a runtime assertion, since a `static_assert` that fires cannot also be @@ -192,5 +202,11 @@ linked into the suite. wizard and stays. - In-flight coalescing. It was removed with the reactive draft in `779bd8aa` and is not reintroduced here; `FlowSession` does without it too. +- **Writing prefill values into drafts.** An earlier draft of this design had + the framework write a bound field when its source fired. That is not parity + with `FlowSession` — it is a mechanism that exists nowhere in the tree, and + would need JSON-to-typed-field deserialization keyed by field name. Rejected + in favour of matching the sibling type; if it is wanted later it is its own + piece of work, and the renderer can do it today from `resolved()`. - A QML renderer for section groups. The schema document is emitted; consuming it is a separate piece of work. From 9e2f45b4bc43001f950c428b532a68346eb7fbbf Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:26:17 +0200 Subject: [PATCH 03/13] docs: implementation plan for unordered sections (#513) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- .../plans/2026-09-10-unordered-sections.md | 1148 +++++++++++++++++ 1 file changed, 1148 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-10-unordered-sections.md diff --git a/docs/superpowers/plans/2026-09-10-unordered-sections.md b/docs/superpowers/plans/2026-09-10-unordered-sections.md new file mode 100644 index 00000000..1dd40c2f --- /dev/null +++ b/docs/superpowers/plans/2026-09-10-unordered-sections.md @@ -0,0 +1,1148 @@ +# Unordered Sections Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or + superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for + tracking. + +**Goal:** Add `morph::forms::SectionSet` — N independently editable action drafts, each gated +on its own `ActionValidator::ready`, dispatched through the handler with no sequencing. Closes morph#513. + +**Architecture:** Extract the machinery `FlowSession` and `SectionSet` share into +`forms/detail/session_common.hpp`, leaving `flows.hpp`'s public names intact. Add `forms/sections.hpp` with the +declaration types (`Section`, `SectionGroup`), the schema emitter, and the runtime session. `SectionSet` mirrors +`FlowSession` minus the current-step constraint. + +**Tech Stack:** C++23, header-only. Catch2 v3 for tests. glaze for JSON. No new dependencies. + +## Global Constraints + +Every task's requirements implicitly include these. They are the gates CI applies. + +- **C++23, header-only.** No new third-party dependencies. +- **Build with clang AND gcc.** GCC has `-Werror=useless-cast`, which clang lacks; a clang-only check misses it. + Presets: `clang-debug`, `gcc-debug`. +- **`-Wdocumentation -Werror` clean.** Every public symbol needs complete `@param`/`@tparam`/`@return`. Never write + a Doxygen command name (`@throws`, `@par`) in prose — even in backticks — it parses as a command. +- **Tree-wide clang-format:** `git ls-files -z '*.hpp' '*.cpp' | xargs -0 clang-format --dry-run -Werror` must be + silent. +- **clang-tidy clean on changed lines**, diffed against `origin/master...HEAD` (not the working tree — that is + empty after a commit). +- **Header ↔ spec sync:** any change under `include/morph/forms/` requires a matching change under + `docs/spec/forms/`. Task 8 satisfies this and must land before the PR opens. +- **Invariant 7:** every test must be verified to FAIL without its implementation. A test that passes either way + measures nothing. +- **`scripts/branch_partial_allowlist.json`** must have every `line` landing on its own `source` text, and every + `line N` in `reason` prose resolving to a real entry. Re-pin if line numbers move. + +--- + +## File Structure + +| File | Responsibility | +|---|---| +| `include/morph/forms/detail/session_common.hpp` *(new)* | `Bind`, `forEachTupleElement`, `forPackElement`, `AllDistinct`, `emitBindsInto` — shared by both session types | +| `include/morph/forms/flows.hpp` *(modify)* | Unchanged public surface; helpers now come from the shared header, `morph::flows::Bind` becomes an alias | +| `include/morph/forms/sections.hpp` *(new)* | `Section`, `SectionGroup`, `SectionGroupTraits`, `BRIDGE_REGISTER_SECTION_GROUP`, `sectionGroupSchemaJson`, `SectionSet` | +| `tests/test_sections.cpp` *(new)* | All nine cases from the spec | +| `tests/CMakeLists.txt` *(modify)* | Register the new test file | +| `docs/spec/forms/sections.md` *(new)* | Design spec for the new surface; also satisfies the spec-sync gate | + +--- + +### Task 1: Extract the shared session helpers + +Pure refactor. `flows.hpp` must behave identically and its existing tests must pass untouched. + +**Files:** +- Create: `include/morph/forms/detail/session_common.hpp` +- Modify: `include/morph/forms/flows.hpp` (remove the moved definitions, add the include and the `Bind` alias) +- Test: `tests/test_flows_apps.cpp` (existing — must pass unchanged) + +**Interfaces:** +- Produces: `morph::forms::Bind` with `static constexpr std::string_view field()` and `path()`; + `morph::forms::detail::forEachTupleElement(Visitor&&)`; + `morph::forms::detail::forPackElement(std::size_t, Visitor&&)`; + `morph::forms::detail::AllDistinct::value`; + `morph::forms::detail::emitBindsInto(glz::generic_u64& node)`. +- Consumes: nothing. + +- [ ] **Step 1: Create the shared header** + +```cpp +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +#include +#include +#include +#include +#include +#include + +#include + +#include "../forms.hpp" + +namespace morph::forms { + +/// @brief One `field -> "."` prefill binding declared on a +/// wizard step or an unordered section. +/// +/// A declaration, not a write. Nothing in the framework assigns a bound field: +/// `wizardSchemaJson`/`sectionGroupSchemaJson` emit it for a renderer, and the +/// captured value is read back through `resolved()`. +/// @tparam Field The action's field name to prefill. +/// @tparam Path Source path, `"."`, into captured values. +template +struct Bind { + /// @brief The action field name this binding fills. + /// @return The declared field name. + [[nodiscard]] static constexpr std::string_view field() noexcept { return Field.view(); } + + /// @brief The source path into captured values. + /// @return The declared `"."` path. + [[nodiscard]] static constexpr std::string_view path() noexcept { return Path.view(); } +}; + +namespace detail { + +/// @brief Invokes `visitor.template operator(), I>()` +/// for every element of @p Tuple, in order. +/// @tparam Tuple A `std::tuple<...>` type (only its element types/arity are used). +/// @tparam Visitor Callable with a `template operator()()`. +/// @param visitor Callable invoked once per tuple element. +template +constexpr void forEachTupleElement(Visitor&& visitor) { + [](std::index_sequence, Visitor&& innerVisitor) { + (innerVisitor.template operator(), I>(), ...); + }(std::make_index_sequence>{}, std::forward(visitor)); +} + +/// @brief Invokes `visitor.template operator()()` for the pack element of +/// `Ts...` at runtime position @p index. A no-op when +/// `index >= sizeof...(Ts)`. +/// @tparam Ts The pack to index into. +/// @tparam Visitor Callable with a `template operator()()`. +/// @param index 0-based position to visit. +/// @param visitor Callable invoked for the element at @p index. +template +constexpr void forPackElement(std::size_t index, Visitor&& visitor) { + std::size_t i = 0; + (void)((i++ == index ? (visitor.template operator()(), true) : false) || ...); +} + +/// @brief Trait: `true` when every type in `Ts...` is pairwise distinct. +/// @tparam Ts Types to check for pairwise distinctness. +template +struct AllDistinct : std::true_type {}; + +/// @brief Recursive case: `T` distinct from every type in `Rest...`, and `Rest...` pairwise distinct. +/// @tparam T The type being checked against `Rest...`. +/// @tparam Rest The remaining types. +template +struct AllDistinct : std::bool_constant<(!std::is_same_v && ...) && AllDistinct::value> { +}; + +/// @brief Writes each `Bind` in @p BindsTuple into @p node as +/// `"": ""`. A no-op for an empty tuple. +/// @tparam BindsTuple `std::tuple...>`. +/// @param node Destination JSON object node. +template +void emitBindsInto(glz::generic_u64& node) { + forEachTupleElement([&]() { + static_cast(J); + node[std::string{BindT::field()}] = std::string{BindT::path()}; + }); +} + +} // namespace detail +} // namespace morph::forms +``` + +- [ ] **Step 2: Point flows.hpp at it** + +In `include/morph/forms/flows.hpp`: add `#include "detail/session_common.hpp"` beside the existing `#include +"forms.hpp"`. Delete the `Bind` struct definition and the `detail` block containing `forEachTupleElement`, +`forStep`, and `AllDistinct`. Add the compatibility alias in `namespace morph::flows`: + +```cpp +/// @brief Prefill binding for a wizard step. +/// +/// Alias for `morph::forms::Bind`, which both session types share. Kept in this +/// namespace because it is the name shipped consumers already write. +/// @tparam Field The step action's field name to prefill. +/// @tparam Path Source path, `"."`, into captured values. +template +using Bind = morph::forms::Bind; +``` + +Then replace the internal call sites: `detail::forEachTupleElement<...>` becomes +`::morph::forms::detail::forEachTupleElement<...>`, `detail::forStep<...>` becomes +`::morph::forms::detail::forPackElement<...>`, and `detail::AllDistinct<...>` becomes +`::morph::forms::detail::AllDistinct<...>`. + +- [ ] **Step 3: Build both compilers and run the existing suite** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +./build/clang-debug/tests/morph_tests "[flows]" +cmake --build build/gcc-net --target morph_tests -j 12 2>&1 | grep -c "error:" +``` + +Expected: build clean under both; `[flows]` cases all pass. This is a refactor — a behaviour change here is a bug. + +- [ ] **Step 4: Commit** + +```bash +git add include/morph/forms/detail/session_common.hpp include/morph/forms/flows.hpp +git commit -m "forms: extract the session helpers both FlowSession and SectionSet need" +``` + +--- + +### Task 2: Declaration types and the schema document + +**Files:** +- Create: `include/morph/forms/sections.hpp` +- Create: `tests/test_sections.cpp` +- Modify: `tests/CMakeLists.txt:102` (add `test_sections.cpp` beside `test_flows_apps.cpp`) + +**Interfaces:** +- Consumes: `morph::forms::Bind`, `morph::forms::detail::forEachTupleElement`, + `morph::forms::detail::emitBindsInto` (Task 1). +- Produces: `morph::forms::Section` with `using action`, `using binds`, `static constexpr + std::string_view title()`; `morph::forms::SectionGroup` with `using sections`, `title()`; + `morph::forms::SectionGroupTraits::typeId()`; `BRIDGE_REGISTER_SECTION_GROUP(G, NAME)`; + `morph::forms::sectionGroupSchemaJson() -> std::string`. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/test_sections.cpp` (create it with this content): + +```cpp +// SPDX-License-Identifier: Apache-2.0 +#include +#include +#include +#include +#include +#include +#include + +// Two independent sections of one screen: a profile block and a preferences +// block. Neither is a step of the other -- editing them in either order is the +// point of SectionSet. +struct SecProfile { + std::string name; + [[nodiscard]] bool validate() const { return !name.empty(); } +}; +struct SecProfileResult { + std::int64_t id = 0; +}; +struct SecPrefs { + std::int64_t profileId = 0; + std::string theme; + [[nodiscard]] bool validate() const { return !theme.empty(); } +}; +struct SecPrefsResult { + std::string summary; +}; + +struct SecModel { + SecProfileResult execute(SecProfile action) { return {.id = static_cast(action.name.size())}; } + SecPrefsResult execute(SecPrefs action) { return {.summary = action.theme}; } +}; + +BRIDGE_REGISTER_MODEL(SecModel, "SectionsTest_Model") +BRIDGE_REGISTER_ACTION(SecModel, SecProfile, "SectionsTest_Profile") +BRIDGE_REGISTER_ACTION(SecModel, SecPrefs, "SectionsTest_Prefs") + +using ProfileSection = morph::forms::Section; +using PrefsSection = + morph::forms::Section>; +using DemoGroup = morph::forms::SectionGroup<"Account settings", ProfileSection, PrefsSection>; +BRIDGE_REGISTER_SECTION_GROUP(DemoGroup, "SectionsTest_DemoGroup") + +TEST_CASE("sectionGroupSchemaJson carries each section's title, action and binds", "[sections][schema]") { + auto const json = morph::forms::sectionGroupSchemaJson(); + REQUIRE_FALSE(json.empty()); + + glz::generic_u64 dom{}; + REQUIRE_FALSE(glz::read_json(dom, json)); + + CHECK(dom["s-id"].get_string() == "SectionsTest_DemoGroup"); + CHECK(dom["s-title"].get_string() == "Account settings"); + + auto const& sections = dom["s-sections"].get_array(); + REQUIRE(sections.size() == 2); + CHECK(sections[0]["action"].get_string() == "SectionsTest_Profile"); + CHECK(sections[0]["title"].get_string() == "Profile"); + CHECK(sections[1]["action"].get_string() == "SectionsTest_Prefs"); + CHECK(sections[1]["prefill"]["profileId"].get_string() == "SectionsTest_Profile.id"); + + // No order is implied: a section carries no index field, because a renderer + // chooses its own arrangement and an emitted position would imply a + // sequence SectionSet does not have. + CHECK_FALSE(sections[0].contains("index")); +} +``` + +- [ ] **Step 2: Register the test and run it to verify it fails** + +Add `test_sections.cpp` to `tests/CMakeLists.txt` beside `test_flows_apps.cpp`, then: + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +``` + +Expected: FAIL to compile — `morph/forms/sections.hpp` does not exist. + +- [ ] **Step 3: Write the declaration types and emitter** + +Create `include/morph/forms/sections.hpp`: + +```cpp +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +/// @file forms/sections.hpp +/// @brief Unordered sections: N independently editable action drafts on one +/// screen, each with its own readiness gate and no sequencing. +/// +/// The sibling to `morph::flows::FlowSession`. A wizard has one active step and +/// refuses a field belonging to any other; a section set has no active step at +/// all. Use this when step order carries no meaning -- tabs, cards, a settings +/// page -- and `FlowSession` when it does. + +#include +#include +#include +#include +#include +#include + +#include + +#include "../core/bridge.hpp" +#include "detail/session_common.hpp" + +namespace morph::forms { + +/// @brief One section of a `SectionGroup`: a registered action, a display +/// title, and zero or more `Bind` prefill declarations. +/// @tparam Action Registered action type (`BRIDGE_REGISTER_ACTION`) this section fires. +/// @tparam Title Human title for the section (tab label / card header). +/// @tparam Binds Zero or more `Bind` prefill declarations. +template +struct Section { + /// @brief The section's action type. + using action = Action; + + /// @brief Tuple of this section's `Bind<...>` declarations (possibly empty). + using binds = std::tuple; + + /// @brief The section's display title. + /// @return The declared title. + [[nodiscard]] static constexpr std::string_view title() noexcept { return Title.view(); } +}; + +/// @brief An unordered set of `Section`s sharing one screen. +/// @tparam Title Human title for the whole group. +/// @tparam Sections One or more `Section` types. +template +struct SectionGroup { + /// @brief Tuple of this group's `Section<...>` types. + using sections = std::tuple; + + /// @brief The group's display title. + /// @return The declared title. + [[nodiscard]] static constexpr std::string_view title() noexcept { return Title.view(); } +}; + +/// @brief Traits specialisation mapping a `SectionGroup` type to its string type-id. +/// +/// Specialise via `BRIDGE_REGISTER_SECTION_GROUP` rather than by hand. The +/// emitted schema needs a stable name for the group; deriving one from the C++ +/// type would tie the wire format to a mangled name. +/// @tparam G The `SectionGroup` type. +template +struct SectionGroupTraits; // forward — specialise or use BRIDGE_REGISTER_SECTION_GROUP + +/// @brief Generates the `s-*` JSON document for section group @p G. +/// +/// Carries the group id and title, and one entry per section with its action +/// type-id, title and declared `prefill` binds. Deliberately emits no index or +/// order field: a renderer chooses its own arrangement, and a position would +/// imply a sequence this type does not have. +/// @tparam G The `SectionGroup` type to describe. +/// @return The JSON document, or an empty string if serialization fails. +template +[[nodiscard]] std::string sectionGroupSchemaJson() { + glz::generic_u64 dom{}; + dom["s-id"] = std::string{SectionGroupTraits::typeId()}; + dom["s-title"] = std::string{G::title()}; + + glz::generic_u64::array_t sections{}; + detail::forEachTupleElement([&]() { + static_cast(I); + glz::generic_u64 entry{}; + entry["action"] = std::string{::morph::model::ActionTraits::typeId()}; + entry["title"] = std::string{SectionT::title()}; + if constexpr (std::tuple_size_v != 0) { + detail::emitBindsInto(entry["prefill"]); + } + sections.emplace_back(std::move(entry)); + }); + dom["s-sections"] = sections; + + return glz::write_json(dom).value_or(std::string{}); +} + +} // namespace morph::forms + +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". +/// @brief Specialises `morph::forms::SectionGroupTraits` with the string type-id @p NAME. +#define BRIDGE_REGISTER_SECTION_GROUP(G, NAME) \ + template <> \ + struct morph::forms::SectionGroupTraits { \ + static constexpr std::string_view typeId() noexcept { return NAME; } \ + }; +// clang-format on +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +./build/clang-debug/tests/morph_tests "[sections][schema]" +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp tests/CMakeLists.txt +git commit -m "forms: Section/SectionGroup declarations and their schema document" +``` + +--- + +### Task 3: SectionSet — independent, unordered firing + +The core of morph#513. Case 1 is the regression test: this shape throws under `FlowSession`. + +**Files:** +- Modify: `include/morph/forms/sections.hpp` (add `SectionSet` before the closing `} // namespace morph::forms`) +- Test: `tests/test_sections.cpp` + +**Interfaces:** +- Consumes: `Section`, `SectionGroup` (Task 2); `morph::forms::detail::AllDistinct` (Task 1). +- Produces: `SectionSet` with `explicit SectionSet(BridgeHandler&, + std::function = nullptr)`, `template void set(ValueType)`, and the + private `fire(A draft)`. + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/test_sections.cpp`: + +```cpp +namespace { +/// Collects the actions the model actually executed, in order. +struct SecRecorder { + std::mutex mtx; + std::vector fired; + + void record(std::string what) { + std::scoped_lock const lock{mtx}; + fired.push_back(std::move(what)); + } + [[nodiscard]] std::vector snapshot() { + std::scoped_lock const lock{mtx}; + return fired; + } +}; +SecRecorder& recorder() { + static SecRecorder inst; + return inst; +} +} // namespace + +TEST_CASE("SectionSet: sections fire independently, in any order", "[sections][morph513]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().fired.clear(); + + // Edit the SECOND section first. Under FlowSession this throws + // std::logic_error -- "field belongs to an action that is not the current + // step" -- which is exactly the gap morph#513 reports. + sections.set<&SecPrefs::theme>("dark"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + + // Then the first. Both fire; neither is "current". + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); + + auto const fired = recorder().snapshot(); + CHECK(fired[0] == "SectionsTest_Prefs"); + CHECK(fired[1] == "SectionsTest_Profile"); +} + +TEST_CASE("SectionSet: a not-ready draft does not dispatch", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().fired.clear(); + + // SecPrefs::validate() requires a non-empty theme; profileId alone is not ready. + sections.set<&SecPrefs::profileId>(7); + CHECK(recorder().snapshot().empty()); + + // Completing it dispatches. + sections.set<&SecPrefs::theme>("light"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); +} + +TEST_CASE("SectionSet: an already-fired section fires again on the next edit", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().fired.clear(); + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + + // No latch: still ready, so it dispatches again. + sections.set<&SecProfile::name>("grace"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); +} +``` + +Add `recorder().record(...)` calls to `SecModel::execute`: + +```cpp +struct SecModel { + SecProfileResult execute(SecProfile action) { + recorder().record("SectionsTest_Profile"); + return {.id = static_cast(action.name.size())}; + } + SecPrefsResult execute(SecPrefs action) { + recorder().record("SectionsTest_Prefs"); + return {.summary = action.theme}; + } +}; +``` + +Move the `SecRecorder` block above `SecModel`, and add these includes: ``, ``, ``, +``, ``, `"test_support.hpp"`. + +- [ ] **Step 2: Run to verify they fail** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +``` + +Expected: FAIL to compile — `SectionSet` is not declared. + +- [ ] **Step 3: Implement SectionSet's core** + +Insert before the closing `} // namespace morph::forms` in `sections.hpp`: + +```cpp +/// @brief Drives N independently editable action drafts on one screen. +/// +/// Each section accumulates its own draft through `set<>`, and dispatches +/// through the handler as soon as `ActionValidator::ready` accepts it. +/// There is no active section and no sequence: a field belonging to any +/// declared section may be set at any time. +/// +/// A ready section re-fires on every subsequent `set<>`. There is no latch and +/// no coalescing, matching `FlowSession`; a caller wanting one request per +/// pause debounces on its own side. +/// @tparam Model The model the handler is bound to. +/// @tparam Sections One or more `Section` types. +template +class SectionSet { + static_assert(sizeof...(Sections) > 0, "SectionSet: a group needs at least one section"); + static_assert(detail::AllDistinct::value, + "SectionSet: section action types must be pairwise distinct"); + +public: + /// @brief Constructs a section set over @p handler. + /// @param handler Handler every section dispatches through. Must outlive + /// this `SectionSet` (only *destruction* order is + /// unconstrained; see bridge.md's Lifetime & ownership). + /// @param onError Optional callback invoked when a section's dispatch + /// fails. When absent the error is logged via + /// `morph::log::logError`. Stored and invoked for this + /// object's whole lifetime, so anything the callable refers + /// to must outlive it. + explicit SectionSet(::morph::bridge::BridgeHandler& handler MORPH_LIFETIMEBOUND, + std::function onError MORPH_LIFETIMEBOUND = nullptr) + : _handler{handler}, _onError{std::move(onError)} {} + + SectionSet(const SectionSet&) = delete; + SectionSet& operator=(const SectionSet&) = delete; + SectionSet(SectionSet&&) = delete; + SectionSet& operator=(SectionSet&&) = delete; + + /// @brief Sets one field of its section's draft and dispatches that section + /// if the draft is now ready. + /// + /// Unlike `FlowSession::set<>` this imposes no ordering: the field's action + /// need only be one of the declared sections. + /// @tparam FieldPtr Pointer-to-data-member of a declared section's action struct. + /// @param value New value for the field. + template + void set(typename ::morph::bridge::detail::MemberPointerTraits::ValueType value) { + using A = typename ::morph::bridge::detail::MemberPointerTraits::ClassType; + static_assert((std::is_same_v || ...), + "SectionSet::set<>: field's action is not a section of this group"); + A draft{}; + { + std::scoped_lock const lock{_mtx}; + std::get(_drafts).*FieldPtr = std::move(value); + draft = std::get(_drafts); + } + if (::morph::model::ActionValidator::ready(draft)) { + fire(std::move(draft)); + } + } + +private: + template + void fire(A draft) { + _handler.execute(std::move(draft)); + } + + ::morph::bridge::BridgeHandler& _handler; + std::function _onError; + mutable std::mutex _mtx; + std::tuple _drafts{}; +}; +``` + +Add `#include ` and `#include ` to the header's include block. + +- [ ] **Step 4: Run the tests to verify they pass** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +./build/clang-debug/tests/morph_tests "[sections]" +``` + +Expected: PASS, all four cases. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp +git commit -m "forms: SectionSet dispatches each section independently (#513)" +``` + +--- + +### Task 4: reset and draft accessors + +**Files:** +- Modify: `include/morph/forms/sections.hpp` +- Test: `tests/test_sections.cpp` + +**Interfaces:** +- Consumes: `SectionSet` (Task 3). +- Produces: `template void reset()`; `template [[nodiscard]] A draft() const`. + +- [ ] **Step 1: Write the failing test** + +```cpp +TEST_CASE("SectionSet: reset clears one section and leaves the others intact", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + sections.set<&SecPrefs::profileId>(42); // not ready: no theme yet + sections.set<&SecProfile::name>("ada"); // ready: fires + + CHECK(sections.draft().profileId == 42); + CHECK(sections.draft().name == "ada"); + + sections.reset(); + + CHECK(sections.draft().profileId == 0); + // The other section is untouched -- per-section isolation is the point. + CHECK(sections.draft().name == "ada"); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +``` + +Expected: FAIL to compile — no member `reset`/`draft`. + +- [ ] **Step 3: Implement them** + +Add to `SectionSet`'s public section, after `set<>`: + +```cpp + /// @brief Clears one section's draft back to a default-constructed action. + /// + /// Touches no other section, and dispatches nothing. + /// @tparam A The section's action type. + template + void reset() { + static_assert((std::is_same_v || ...), + "SectionSet::reset<>: not a section of this group"); + std::scoped_lock const lock{_mtx}; + std::get(_drafts) = A{}; + } + + /// @brief Snapshots one section's current draft. + /// + /// A copy taken under the lock, not a reference into live state, so a + /// renderer can read it while another thread edits a different section. + /// @tparam A The section's action type. + /// @return The draft as it stands. + template + [[nodiscard]] A draft() const { + static_assert((std::is_same_v || ...), + "SectionSet::draft<>: not a section of this group"); + std::scoped_lock const lock{_mtx}; + return std::get(_drafts); + } +``` + +- [ ] **Step 4: Run to verify it passes** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 && ./build/clang-debug/tests/morph_tests "[sections]" +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp +git commit -m "forms: SectionSet reset() and draft()" +``` + +--- + +### Task 5: Result capture and resolved() + +**Files:** +- Modify: `include/morph/forms/sections.hpp` +- Test: `tests/test_sections.cpp` + +**Interfaces:** +- Consumes: `SectionSet::fire` (Task 3). +- Produces: `[[nodiscard]] std::optional resolved(std::string_view path) const`. + +- [ ] **Step 1: Write the failing test** + +```cpp +TEST_CASE("SectionSet: a fired section's fields are resolvable; an unfired one is not", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().fired.clear(); + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + + // The result's field: SecProfileResult::id == name.size() == 3. + CHECK(sections.resolved("SectionsTest_Profile.id") == std::string{"3"}); + // The submitted draft's field is captured too. + CHECK(sections.resolved("SectionsTest_Profile.name") == std::string{"\"ada\""}); + + // Both halves matter: a path whose section never fired resolves to nothing. + // Without this, an implementation returning a value for everything passes. + CHECK_FALSE(sections.resolved("SectionsTest_Prefs.summary").has_value()); + CHECK_FALSE(sections.resolved("SectionsTest_Profile.nosuchfield").has_value()); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +``` + +Expected: FAIL to compile — no member `resolved`. + +- [ ] **Step 3: Implement capture and lookup** + +Replace `fire` and add `captureResult` plus the public `resolved`: + +```cpp + /// @brief Looks up a value captured from a section's submitted draft or result. + /// @param path `"."`, matching a section's declared `Bind::path()`. + /// @return The field's JSON-encoded value, or `std::nullopt` if @p path was + /// never captured (the section never fired, or has no such field). + [[nodiscard]] std::optional resolved(std::string_view path) const { + std::scoped_lock const lock{_mtx}; + auto iter = _resolvedValues.find(std::string{path}); + if (iter == _resolvedValues.end()) { + return std::nullopt; + } + return iter->second; + } +``` + +```cpp + template + void captureResult(const ::morph::model::ActionTraits::Result& result) { + std::scoped_lock const lock{_mtx}; + auto const typeId = ::morph::model::ActionTraits::typeId(); + auto record = [&](const auto& value) { + ::morph::forms::detail::forEachNamedMember( + value, [&](std::string_view name, const auto& member) { + static_cast(I); + std::string json; + if (!glz::write_json(member, json)) { + _resolvedValues[std::string{typeId} + "." + std::string{name}] = std::move(json); + } + }); + }; + record(std::get(_drafts)); // submitted draft fields first... + record(result); // ...result fields win on name collision + } + + template + void fire(A draft) { + _handler.execute(std::move(draft)).then(_callbacks, [this](::morph::model::ActionTraits::Result result) { + this->template captureResult(result); + }); + } +``` + +Add the member `std::unordered_map _resolvedValues;` and `::morph::async::CallbackScope +_callbacks;` (declared last), plus includes `` and ``. + +**Note on ordering:** `_callbacks` must be the last member so it is destroyed last; Task 7 adds the destructor that +stops it first. + +- [ ] **Step 4: Run to verify it passes** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 && ./build/clang-debug/tests/morph_tests "[sections]" +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp +git commit -m "forms: SectionSet captures results and exposes resolved()" +``` + +--- + +### Task 6: Error routing + +**Files:** +- Modify: `include/morph/forms/sections.hpp` +- Test: `tests/test_sections.cpp` + +**Interfaces:** +- Consumes: `SectionSet::fire` (Task 5). +- Produces: the `onError` path and `logUnhandledError`. + +- [ ] **Step 1: Write the failing test** + +Add a throwing action to the fixture: + +```cpp +struct SecExplodes { + std::string label; + [[nodiscard]] bool validate() const { return !label.empty(); } +}; +struct SecExplodesResult { + std::int64_t id = 0; +}; +``` + +Add to `SecModel`: `SecExplodesResult execute(SecExplodes) { throw std::runtime_error{"section boom"}; }`, register +it with `BRIDGE_REGISTER_ACTION(SecModel, SecExplodes, "SectionsTest_Explodes")`, and declare `using +ExplodesSection = morph::forms::Section;`. + +```cpp +TEST_CASE("SectionSet: a failing dispatch reaches the onError callback", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic errors{0}; + morph::forms::SectionSet sections{ + handler, [&](std::exception_ptr) { errors.fetch_add(1); }}; + + sections.set<&SecExplodes::label>("boom"); + REQUIRE(morph::testing::waitUntil([&] { return errors.load() == 1; })); + + // A section that succeeds does not route to onError. + sections.set<&SecProfile::name>("ada"); + CHECK(errors.load() == 1); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 && ./build/clang-debug/tests/morph_tests "a failing dispatch reaches" +``` + +Expected: FAIL — `errors` stays 0, because `fire` installs no `.onError` continuation. + +- [ ] **Step 3: Implement error routing** + +```cpp + static void logUnhandledError(std::string_view typeId, const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + ::morph::log::logError(std::string{"[sections:"} + std::string{typeId} + "] " + exc.what()); + } catch (...) { + ::morph::log::logError(std::string{"[sections:"} + std::string{typeId} + "] unknown exception"); + } + } + + template + void fire(A draft) { + _handler.execute(std::move(draft)) + .then(_callbacks, [this](::morph::model::ActionTraits::Result result) { + this->template captureResult(result); + }) + .onError(_callbacks, [this](const std::exception_ptr& err) { + if (_onError) { + _onError(err); + return; + } + logUnhandledError(::morph::model::ActionTraits::typeId(), err); + }); + } +``` + +Add `#include ` and `#include `. + +- [ ] **Step 4: Run to verify it passes** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 && ./build/clang-debug/tests/morph_tests "[sections]" +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp +git commit -m "forms: SectionSet routes a failed dispatch to onError, else logs" +``` + +--- + +### Task 7: Lifetime — the callback gate + +**Files:** +- Modify: `include/morph/forms/sections.hpp` +- Test: `tests/test_sections.cpp` + +**Interfaces:** +- Consumes: `_callbacks` (Task 5). +- Produces: `~SectionSet()`. + +- [ ] **Step 1: Write the failing test** + +```cpp +TEST_CASE("SectionSet: destroying it with a dispatch in flight delivers nothing", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic errors{0}; + { + morph::forms::SectionSet sections{ + handler, [&](std::exception_ptr) { errors.fetch_add(1); }}; + sections.set<&SecExplodes::label>("boom"); + // Leaves scope with the dispatch possibly still in flight. The + // destructor stops the scope, so no continuation may run against the + // destroyed object. Same-thread delivery via InlineExecutor makes + // check-then-run atomic here -- see callback_scope.md's boundary. + } + // Nothing may arrive after destruction. A count of 1 recorded *before* the + // scope closed is possible and fine; what must not happen is a crash or a + // later increment. + auto const after = errors.load(); + CHECK(morph::testing::waitUntil([&] { return errors.load() == after; })); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 +``` + +Expected: FAIL to compile only if the destructor is missing entirely; otherwise this case documents the gate. If it +compiles and passes without a destructor, that is because `_callbacks`' own destruction happens to cover it — add +the destructor anyway per Step 3 and keep the case as the regression guard for a future pumping teardown. + +- [ ] **Step 3: Add the destructor** + +```cpp + /// @brief Stops every callback this set installed from being delivered. + /// + /// There is nothing to detach: a section's continuations are owned by the + /// in-flight dispatch, not held in a map this object could remove itself + /// from. `_callbacks` gates each one on a token it checks before touching + /// `this`. + /// + /// `requestStop()` is called explicitly rather than left to the member's + /// own destruction, even though `_callbacks` is declared last: members are + /// destroyed only *after* the destructor body, so a body that later grew a + /// call pumping an event loop would otherwise deliver into a half-dead + /// object. The body does not do that today; stopping first keeps it correct + /// if one is added. + /// + /// The strength of the gate depends on which thread destroys this object, + /// exactly as `CallbackScope`'s "Boundary of the guarantee" describes. + /// Destroying it off the delivery thread is advisory only, and that caller + /// owns its own synchronisation. + ~SectionSet() { _callbacks.requestStop(); } +``` + +- [ ] **Step 4: Run the full sections suite** + +```bash +cmake --build build/clang-debug --target morph_tests -j 12 && ./build/clang-debug/tests/morph_tests "[sections]" +``` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add include/morph/forms/sections.hpp tests/test_sections.cpp +git commit -m "forms: SectionSet stops its callback scope before teardown" +``` + +--- + +### Task 8: Spec document and the full gate run + +Required: the Header ↔ spec sync gate fails a PR that changes `include/morph/forms/` without changing +`docs/spec/forms/`. + +**Files:** +- Create: `docs/spec/forms/sections.md` +- Modify: `scripts/branch_partial_allowlist.json` (only if re-pinning is needed) + +- [ ] **Step 1: Write the spec document** + +`docs/spec/forms/sections.md` covering, in this order: what `SectionSet` is and when to choose it over +`FlowSession`; the declaration types; `set`/`reset`/`draft`/`resolved` with their contracts; the no-latch rule and +why; that binds are declarations the renderer acts on, never framework writes; the `s-*` schema shape and the +deliberate absence of an order field; concurrency and the callback-scope boundary; and the two `static_assert`s +(duplicate action types, and a field outside the group) as the compile-time contract, with a short negative example +for each. + +- [ ] **Step 2: Run every gate CI applies** + +```bash +# format, whole tree +git ls-files -z '*.hpp' '*.cpp' | xargs -0 clang-format --dry-run -Werror + +# documentation warnings, glaze as a system header the way CI treats it +clang++ -std=c++23 -fsyntax-only -Wdocumentation -Wno-documentation-unknown-command -Werror \ + -Iinclude -isystem build/clang-coverage/_deps/glaze-src/include \ + <(printf '#include \nint main(){}\n') -x c++ - + +# both compilers +cmake --build build/clang-debug --target morph_tests -j 12 +cmake --build build/gcc-net --target morph_tests -j 12 + +# suites +./build/clang-debug/tests/morph_tests +./build/clang-debug/tests/net/morph_net_tests + +# spec citations +bash scripts/check_spec_citations.sh + +# allowlist, both ways +python3 -c " +import json,pathlib,re +d=json.load(open('scripts/branch_partial_allowlist.json')); byline={e['line'] for e in d['entries']}; bad=0 +for e in d['entries']: + lines=pathlib.Path(e['file']).read_text().split(chr(10)) + if lines[e['line']-1].strip()!=e['source'].strip(): bad+=1; print('SOURCE',e['file'],e['line']) + for r in re.findall(r'(?:line |\.hpp:)([0-9]+)', e['reason']): + if int(r) not in byline: bad+=1; print('PROSE',e['file'],e['line'],r) +print(f'{len(d[\"entries\"])} entries, {bad} stale')" +``` + +Expected: format silent, no documentation warnings, both builds clean, both suites passing, spec lint OK, allowlist +0 stale. + +- [ ] **Step 3: clang-tidy on the branch's changed lines** + +```bash +git diff -U0 origin/master...HEAD -- '*.cpp' '*.hpp' > /tmp/br.diff +# Extract changed line numbers, run clang-tidy on changed .cpp files, and report +# only findings whose file:line appears in the diff. Diff against +# origin/master...HEAD, NOT the working tree -- the working tree is empty after +# a commit, which makes the filter silently vacuous. +``` + +Expected: no findings on changed lines. + +- [ ] **Step 4: Verify every test fails without its implementation** + +For each case added in Tasks 2-7, stash the corresponding part of `sections.hpp`, rebuild, and confirm the case +fails. A case that passes with the implementation removed is not a test. + +- [ ] **Step 5: Commit and open the PR** + +```bash +git add docs/spec/forms/sections.md scripts/branch_partial_allowlist.json +git commit -m "docs: spec for unordered sections (#513)" +``` + +PR body must state: what SectionSet is, that case 1 is the morph#513 regression, the measured before/after for each +test, and that binds are declarations rather than writes (with the reason, since the first design got this wrong). + +--- + +## Self-Review + +**Spec coverage.** Every section of `2026-09-10-unordered-sections-design.md` maps to a task: Headers → Task 1-2; +Declaration layer → Task 2; SectionSet surface → Tasks 3-5; Prefill → Tasks 2 (schema) and 5 (resolved); +Concurrency and lifetime → Tasks 5, 7; Schema document → Task 2; Testing cases 1-8 → Tasks 2-7; case 9 +(`static_assert`s) → Task 8's spec document, as the spec says it is compile-time and covered by documented negative +examples. + +**Placeholders.** None. Every code step carries the code. + +**Type consistency.** `Section::action`/`binds`/`title()`, `SectionGroup::sections`/`title()`, +`SectionGroupTraits::typeId()`, `sectionGroupSchemaJson()`, and `SectionSet`'s +`set`/`reset`/`draft`/`resolved`/`fire`/`captureResult`/`logUnhandledError` are spelled identically in every task +that references them. `forStep` is renamed to `forPackElement` in Task 1 and used under that name thereafter. From 4cb331a3acb0ad2802af8802929585fda010b9bf Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:31:58 +0200 Subject: [PATCH 04/13] forms: extract the session helpers both FlowSession and SectionSet need `Bind`, the tuple/pack walkers and the distinctness trait were written inside `morph::flows` but describe how a form session declares its units, not how a wizard sequences them. Move them to `morph/forms/detail/session_common.hpp`; `morph::flows::Bind` stays as an alias, since that is the name consumers write. app.hpp reached into `flows::detail` for a walker it uses on its own menu and screens tuples -- nothing to do with wizards. It now includes the shared header directly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/forms/app.hpp | 5 +- include/morph/forms/detail/session_common.hpp | 100 ++++++++++++++++++ include/morph/forms/flows.hpp | 72 +++---------- 3 files changed, 115 insertions(+), 62 deletions(-) create mode 100644 include/morph/forms/detail/session_common.hpp diff --git a/include/morph/forms/app.hpp b/include/morph/forms/app.hpp index 23f25cdf..d4581182 100644 --- a/include/morph/forms/app.hpp +++ b/include/morph/forms/app.hpp @@ -18,6 +18,7 @@ #include #include +#include "detail/session_common.hpp" #include "flows.hpp" namespace morph::app { @@ -125,7 +126,7 @@ template dom["app-title"] = std::string{AppT::title()}; glz::generic_u64::array_t menu{}; - ::morph::flows::detail::forEachTupleElement([&]() { + ::morph::forms::detail::forEachTupleElement([&]() { static_cast(I); glz::generic_u64 entry{}; entry["label"] = std::string{Entry::label()}; @@ -135,7 +136,7 @@ template dom["app-menu"] = menu; auto& screensNode = dom["app-screens"]; - ::morph::flows::detail::forEachTupleElement([&]() { + ::morph::forms::detail::forEachTupleElement([&]() { static_cast(I); auto& screenNode = screensNode[std::string{S::id()}]; screenNode["kind"] = std::string{S::kind()}; diff --git a/include/morph/forms/detail/session_common.hpp b/include/morph/forms/detail/session_common.hpp new file mode 100644 index 00000000..940187de --- /dev/null +++ b/include/morph/forms/detail/session_common.hpp @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +/// @file forms/detail/session_common.hpp +/// @brief Machinery shared by the two form session types: `morph::flows::FlowSession` +/// (ordered wizard steps) and `morph::forms::SectionSet` (unordered sections). +/// +/// Both declare their units the same way — a registered action, a title, and +/// zero or more `Bind` prefill declarations — and both walk those declarations +/// to emit a schema document. Only the sequencing differs, so the declaration +/// vocabulary and the pack/tuple walkers live here rather than being written +/// twice. + +#include +#include +#include +#include +#include +#include +#include + +#include "../forms.hpp" + +namespace morph::forms { + +/// @brief One `field -> "."` prefill binding declared on a +/// wizard step or an unordered section. +/// +/// A declaration, not a write. Nothing in the framework assigns a bound field: +/// `wizardSchemaJson`/`sectionGroupSchemaJson` emit it for a renderer, and the +/// captured value is read back through `resolved()`. +/// @tparam Field The action's field name to prefill. +/// @tparam Path Source path, `"."`, into captured values. +template +struct Bind { + /// @brief The action field name this binding fills. + /// @return The declared field name. + [[nodiscard]] static constexpr std::string_view field() noexcept { return Field.view(); } + + /// @brief The source path into captured values. + /// @return The declared `"."` path. + [[nodiscard]] static constexpr std::string_view path() noexcept { return Path.view(); } +}; + +namespace detail { + +/// @brief Invokes `visitor.template operator(), I>()` +/// for every element of @p Tuple, in order. +/// @tparam Tuple A `std::tuple<...>` type (only its element types/arity are used). +/// @tparam Visitor Callable with a `template operator()()`. +/// @param visitor Callable invoked once per tuple element. +template +constexpr void forEachTupleElement(Visitor&& visitor) { + [](std::index_sequence, Visitor&& innerVisitor) { + (innerVisitor.template operator(), I>(), ...); + }(std::make_index_sequence>{}, std::forward(visitor)); +} + +/// @brief Invokes `visitor.template operator()()` for the pack element of +/// `Ts...` at runtime position @p index. A no-op when +/// `index >= sizeof...(Ts)`. +/// @tparam Ts The pack to index into. +/// @tparam Visitor Callable with a `template operator()()`. +/// @param index 0-based position to visit. +/// @param visitor Callable invoked for the element at @p index. +template +constexpr void forPackElement(std::size_t index, Visitor&& visitor) { + std::size_t i = 0; + (void)((i++ == index ? (visitor.template operator()(), true) : false) || ...); +} + +/// @brief Trait: `true` when every type in `Ts...` is pairwise distinct. +/// @tparam Ts Types to check for pairwise distinctness. +template +struct AllDistinct : std::true_type {}; + +/// @brief Recursive case: `T` distinct from every type in `Rest...`, and `Rest...` pairwise distinct. +/// @tparam T The type being checked against `Rest...`. +/// @tparam Rest The remaining types. +template +struct AllDistinct : std::bool_constant<(!std::is_same_v && ...) && AllDistinct::value> { +}; + +/// @brief Writes each `Bind` in @p BindsTuple into @p node as `"": ""`. +/// +/// A no-op for an empty tuple, so callers guard on arity only to avoid +/// materialising an empty `prefill` object. +/// @tparam BindsTuple `std::tuple...>`. +/// @param node Destination JSON object node. +template +void emitBindsInto(glz::generic_u64& node) { + forEachTupleElement([&]() { + static_cast(J); + node[std::string{BindT::field()}] = std::string{BindT::path()}; + }); +} + +} // namespace detail +} // namespace morph::forms diff --git a/include/morph/forms/flows.hpp b/include/morph/forms/flows.hpp index 16995668..2c4c0032 100644 --- a/include/morph/forms/flows.hpp +++ b/include/morph/forms/flows.hpp @@ -41,25 +41,20 @@ #include "../core/bridge.hpp" #include "../core/callback_scope.hpp" #include "../core/logger.hpp" +#include "detail/session_common.hpp" #include "forms.hpp" namespace morph::flows { -/// @brief One `field -> "."` prefill binding declared on -/// a wizard step. +/// @brief Prefill binding for a wizard step. +/// +/// Alias for `morph::forms::Bind`, which both session types share. Kept in this +/// namespace because it is the name shipped consumers already write. /// @tparam Field The step action's field name to prefill. /// @tparam Path Source path, `"."`, into an earlier /// step's captured values (see `FlowSession::resolved`). template -struct Bind { - /// @brief The step action's field name this binding fills. - /// @return The declared field name. - [[nodiscard]] static constexpr std::string_view field() noexcept { return Field.view(); } - - /// @brief The source path into an earlier step's captured values. - /// @return The declared `"."` path. - [[nodiscard]] static constexpr std::string_view path() noexcept { return Path.view(); } -}; +using Bind = morph::forms::Bind; /// @brief One step of a `Wizard`: a registered action, a display title, and /// zero or more `Bind` prefill declarations. @@ -101,47 +96,6 @@ struct Wizard { template struct WizardTraits; // forward — specialise or use BRIDGE_REGISTER_WIZARD -namespace detail { - -/// @brief Invokes `visitor.template operator(), I>()` -/// for every element of @p Tuple, in order. -/// @tparam Tuple A `std::tuple<...>` type (only its element types/arity are used). -/// @tparam Visitor Callable with a `template operator()()`. -/// @param visitor Callable invoked once per tuple element. -template -constexpr void forEachTupleElement(Visitor&& visitor) { - [](std::index_sequence, Visitor&& innerVisitor) { - (innerVisitor.template operator(), I>(), ...); - }(std::make_index_sequence>{}, std::forward(visitor)); -} - -/// @brief Invokes `visitor.template operator()()` for the pack element -/// of `Steps...` at runtime position @p index. A no-op when -/// `index >= sizeof...(Steps)`. -/// @tparam Steps The pack to index into. -/// @tparam Visitor Callable with a `template operator()()`. -/// @param index 0-based position to visit. -/// @param visitor Callable invoked for the step at @p index. -template -constexpr void forStep(std::size_t index, Visitor&& visitor) { - std::size_t i = 0; - (void)((i++ == index ? (visitor.template operator()(), true) : false) || ...); -} - -/// @brief Trait: `true` when every type in `Ts...` is pairwise distinct. -/// @tparam Ts Types to check for pairwise distinctness. -template -struct AllDistinct : std::true_type {}; - -/// @brief Recursive case: `T` distinct from every type in `Rest...`, and `Rest...` pairwise distinct. -/// @tparam T The type being checked against `Rest...`. -/// @tparam Rest The remaining types. -template -struct AllDistinct : std::bool_constant<(!std::is_same_v && ...) && AllDistinct::value> { -}; - -} // namespace detail - /// @brief Generates the `w-*` JSON document for wizard type @p W. /// /// Emits `w-title` and an ordered `w-steps` array; each step carries `action` @@ -158,17 +112,13 @@ template dom["w-title"] = std::string{W::title()}; glz::generic_u64::array_t steps{}; - detail::forEachTupleElement([&]() { + ::morph::forms::detail::forEachTupleElement([&]() { static_cast(I); glz::generic_u64 step{}; step["action"] = std::string{::morph::model::ActionTraits::typeId()}; step["title"] = std::string{StepT::title()}; if constexpr (std::tuple_size_v != 0) { - auto& prefillNode = step["prefill"]; - detail::forEachTupleElement([&]() { - static_cast(J); - prefillNode[std::string{BindT::field()}] = std::string{BindT::path()}; - }); + ::morph::forms::detail::emitBindsInto(step["prefill"]); } steps.emplace_back(std::move(step)); }); @@ -199,7 +149,8 @@ template template class FlowSession { static_assert(sizeof...(Steps) > 0, "FlowSession: a flow needs at least one step"); - static_assert(detail::AllDistinct::value, "FlowSession: step action types must be pairwise distinct"); + static_assert(::morph::forms::detail::AllDistinct::value, + "FlowSession: step action types must be pairwise distinct"); public: /// @brief Constructs a flow over @p handler, starting at step 0. @@ -406,7 +357,8 @@ class FlowSession { /// @return Empty when `finished()` (no current step). [[nodiscard]] std::string_view currentActionType() const noexcept { std::string_view id{}; - detail::forStep(_index, [&id] { id = ::morph::model::ActionTraits::typeId(); }); + ::morph::forms::detail::forPackElement( + _index, [&id] { id = ::morph::model::ActionTraits::typeId(); }); return id; } From 064a4ee69b8f93669c991378e29d3ce5b5096a00 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:34:18 +0200 Subject: [PATCH 05/13] forms: Section/SectionGroup declarations and their schema document The declaration half of unordered sections (#513): a Section names a registered action, a title and its Bind prefills; a SectionGroup collects them. sectionGroupSchemaJson emits the s-* document a renderer consumes. The document carries no index or order field. A renderer arranges the sections itself, and a position in the wire format would suggest a sequence these do not have -- that distinction is the entire reason this exists beside Wizard. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- CMakeLists.txt | 1 + include/morph/forms/sections.hpp | 126 ++++++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/test_sections.cpp | 130 +++++++++++++++++++++++++++++++ 4 files changed, 258 insertions(+) create mode 100644 include/morph/forms/sections.hpp create mode 100644 tests/test_sections.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 1cf24690..aa5c5c99 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -225,6 +225,7 @@ target_sources(morph include/morph/forms/app.hpp include/morph/forms/flows.hpp include/morph/forms/layout.hpp + include/morph/forms/sections.hpp include/morph/forms/views.hpp include/morph/forms/widget_hints.hpp include/morph/journal/outbox.hpp diff --git a/include/morph/forms/sections.hpp b/include/morph/forms/sections.hpp new file mode 100644 index 00000000..d07ac440 --- /dev/null +++ b/include/morph/forms/sections.hpp @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +/// @file forms/sections.hpp +/// @brief Unordered sections: N independently editable action drafts on one +/// screen, each with its own readiness gate and no sequencing. +/// +/// The sibling to `morph::flows::FlowSession`. A wizard has one active step and +/// throws on a field belonging to any other; a section set has no active +/// section at all -- every declared section is editable at every moment, and +/// each dispatches on its own as soon as its draft validates. +/// +/// Choose this when order carries no meaning (tabs, cards, a settings page) and +/// `FlowSession` when it does. Like flows, this is additive metadata and +/// dispatch bookkeeping over the ordinary `BridgeHandler::execute()` +/// path: no new wire format, no new execution mode. +/// See docs/spec/forms/sections.md. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../attributes.hpp" +#include "../core/bridge.hpp" +#include "../core/callback_scope.hpp" +#include "../core/logger.hpp" +#include "detail/session_common.hpp" + +namespace morph::forms { + +/// @brief One section of a `SectionGroup`: a registered action, a display +/// title, and zero or more `Bind` prefill declarations. +/// @tparam Action Registered action type (`BRIDGE_REGISTER_ACTION`) this section fires. +/// @tparam Title Human title for the section (tab label / card header). +/// @tparam Binds Zero or more `Bind` prefill declarations. +template +struct Section { + /// @brief The section's action type. + using action = Action; + + /// @brief Tuple of this section's `Bind<...>` prefill declarations (possibly empty). + using binds = std::tuple; + + /// @brief The section's display title. + /// @return The declared title. + [[nodiscard]] static constexpr std::string_view title() noexcept { return Title.view(); } +}; + +/// @brief An unordered set of `Section`s sharing one screen. +/// @tparam Title Human title for the whole group. +/// @tparam Sections One or more `Section` types. +template +struct SectionGroup { + /// @brief Tuple of this group's `Section<...>` types. + using sections = std::tuple; + + /// @brief The group's display title. + /// @return The declared title. + [[nodiscard]] static constexpr std::string_view title() noexcept { return Title.view(); } +}; + +/// @brief Traits specialisation mapping a `SectionGroup` type to its string type-id. +/// +/// Specialise via `BRIDGE_REGISTER_SECTION_GROUP` rather than by hand. The +/// default is a forward declaration — using it without a specialisation is an +/// incomplete-type error. The schema needs a stable name for the group; +/// deriving one from the C++ type would tie the wire format to a mangled name. +/// @tparam G Concrete `SectionGroup<...>` type. +template +struct SectionGroupTraits; // forward — specialise or use BRIDGE_REGISTER_SECTION_GROUP + +/// @brief Generates the `s-*` JSON document for section group @p G. +/// +/// Emits `s-id`, `s-title`, and an `s-sections` array; each entry carries +/// `action` (the section's registered action type-id), `title`, and — only when +/// the section declares at least one `Bind` — a `prefill` object mapping field +/// name to `"."` path. +/// +/// Deliberately emits no index or order field. A renderer arranges the sections +/// itself, and a position in the document would suggest a sequence this type +/// does not have. +/// @tparam G Concrete `SectionGroup` type. +/// @return The group's JSON document. Empty string only if glaze's own JSON +/// writer fails on the assembled DOM (schema generation never throws). +template +[[nodiscard]] std::string sectionGroupSchemaJson() { + glz::generic_u64 dom{}; + dom["s-id"] = std::string{SectionGroupTraits::typeId()}; + dom["s-title"] = std::string{G::title()}; + + glz::generic_u64::array_t sections{}; + detail::forEachTupleElement([&]() { + static_cast(I); + glz::generic_u64 entry{}; + entry["action"] = std::string{::morph::model::ActionTraits::typeId()}; + entry["title"] = std::string{SectionT::title()}; + if constexpr (std::tuple_size_v != 0) { + detail::emitBindsInto(entry["prefill"]); + } + sections.emplace_back(std::move(entry)); + }); + dom["s-sections"] = sections; + + return glz::write_json(dom).value_or(std::string{}); +} + +} // namespace morph::forms + +// clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". +/// @brief Specialises `morph::forms::SectionGroupTraits` with the string type-id @p NAME. +#define BRIDGE_REGISTER_SECTION_GROUP(G, NAME) \ + template <> \ + struct morph::forms::SectionGroupTraits { \ + static constexpr std::string_view typeId() noexcept { return NAME; } \ + }; +// clang-format on diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 054f1574..93d8ef24 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -100,6 +100,7 @@ add_executable(morph_tests test_quantity_decode_validation.cpp test_nested_forms.cpp test_flows_apps.cpp + test_sections.cpp test_views.cpp test_computed_fields.cpp test_forms_rules.cpp diff --git a/tests/test_sections.cpp b/tests/test_sections.cpp new file mode 100644 index 00000000..4b11d080 --- /dev/null +++ b/tests/test_sections.cpp @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "test_support.hpp" + +// --------------------------------------------------------------------------- +// Fixture: two independent sections of one screen -- a profile block and a +// preferences block. Neither is a step of the other. Editing them in either +// order is the whole point of SectionSet, and is what FlowSession refuses. +// --------------------------------------------------------------------------- + +namespace { +/// Records which actions the model actually executed, in order. +struct SecRecorder { + std::mutex mtx; + std::vector fired; + + void record(std::string what) { + std::scoped_lock const lock{mtx}; + fired.push_back(std::move(what)); + } + void clear() { + std::scoped_lock const lock{mtx}; + fired.clear(); + } + [[nodiscard]] std::vector snapshot() { + std::scoped_lock const lock{mtx}; + return fired; + } +}; +SecRecorder& recorder() { + static SecRecorder inst; + return inst; +} +} // namespace + +struct SecProfile { + std::string name; + [[nodiscard]] bool validate() const { return !name.empty(); } +}; +struct SecProfileResult { + std::int64_t id = 0; +}; + +struct SecPrefs { + std::int64_t profileId = 0; + std::string theme; + [[nodiscard]] bool validate() const { return !theme.empty(); } +}; +struct SecPrefsResult { + std::string summary; +}; + +struct SecExplodes { + std::string label; + [[nodiscard]] bool validate() const { return !label.empty(); } +}; +struct SecExplodesResult { + std::int64_t id = 0; +}; + +struct SecModel { + SecProfileResult execute(SecProfile action) { + recorder().record("SectionsTest_Profile"); + return {.id = static_cast(action.name.size())}; + } + SecPrefsResult execute(SecPrefs action) { + recorder().record("SectionsTest_Prefs"); + return {.summary = action.theme}; + } + SecExplodesResult execute(SecExplodes) { + recorder().record("SectionsTest_Explodes"); + throw std::runtime_error{"section boom"}; + } +}; + +BRIDGE_REGISTER_MODEL(SecModel, "SectionsTest_Model") +BRIDGE_REGISTER_ACTION(SecModel, SecProfile, "SectionsTest_Profile") +BRIDGE_REGISTER_ACTION(SecModel, SecPrefs, "SectionsTest_Prefs") +BRIDGE_REGISTER_ACTION(SecModel, SecExplodes, "SectionsTest_Explodes") + +using ProfileSection = morph::forms::Section; +using PrefsSection = + morph::forms::Section>; +using ExplodesSection = morph::forms::Section; + +using DemoGroup = morph::forms::SectionGroup<"Account settings", ProfileSection, PrefsSection>; +BRIDGE_REGISTER_SECTION_GROUP(DemoGroup, "SectionsTest_DemoGroup") + +TEST_CASE("sectionGroupSchemaJson carries each section's title, action and binds", "[sections]") { + auto const json = morph::forms::sectionGroupSchemaJson(); + REQUIRE_FALSE(json.empty()); + + glz::generic_u64 dom{}; + REQUIRE_FALSE(glz::read_json(dom, json)); + + CHECK(json.contains(R"("s-id":"SectionsTest_DemoGroup")")); + CHECK(json.contains(R"("s-title":"Account settings")")); + CHECK(json.contains(R"("action":"SectionsTest_Profile")")); + CHECK(json.contains(R"("title":"Profile")")); + CHECK(json.contains(R"("action":"SectionsTest_Prefs")")); + CHECK(json.contains(R"("prefill":{"profileId":"SectionsTest_Profile.id"})")); + + // A section with no Bind carries no prefill key at all, rather than an + // empty object a renderer would have to special-case. + auto const& sections = dom["s-sections"].get_array(); + REQUIRE(sections.size() == 2); + CHECK_FALSE(sections[0].contains("prefill")); + + // No order is implied: a section carries no index field, because a renderer + // chooses its own arrangement and an emitted position would suggest a + // sequence a SectionSet does not have. + CHECK_FALSE(sections[0].contains("index")); + CHECK_FALSE(sections[1].contains("index")); +} From 26982ae633cc4164d8002b39afebe621366695ec Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:36:19 +0200 Subject: [PATCH 06/13] forms: SectionSet dispatches each section independently (#513) FlowSession::set<> throws std::logic_error on a field belonging to any step but the current one, which makes it unusable for a screen whose blocks have no order -- tabs, cards, a settings page. That is morph#513. SectionSet keeps FlowSession's per-action draft accumulation and readiness gate and drops the position: every declared section is editable at every moment and dispatches on its own the instant its draft validates. Nothing is keyed to a "current" section, so a late reply cannot be stale -- there is no position for it to be stale relative to. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/forms/sections.hpp | 211 +++++++++++++++++++++++++++++++ tests/test_sections.cpp | 66 ++++++++++ 2 files changed, 277 insertions(+) diff --git a/include/morph/forms/sections.hpp b/include/morph/forms/sections.hpp index d07ac440..cb28a3a9 100644 --- a/include/morph/forms/sections.hpp +++ b/include/morph/forms/sections.hpp @@ -114,6 +114,217 @@ template return glz::write_json(dom).value_or(std::string{}); } +/// @brief Drives N independently editable action drafts on one screen. +/// +/// Each section accumulates its own draft through `set<>` and dispatches +/// through the handler as soon as `ActionValidator::ready` accepts it. +/// There is no active section and no sequence: a field belonging to any +/// declared section may be set at any time, in any order. This is the whole +/// difference from `morph::flows::FlowSession`, which throws on a field +/// outside its current step. +/// +/// A ready section re-fires on every subsequent `set<>`. There is no latch and +/// no coalescing, matching `FlowSession`; a caller that wants one request per +/// pause debounces on its own side, where it knows what a pause means. +/// @tparam Model The model the handler is bound to. +/// @tparam Sections One or more `Section` types. +template +class SectionSet { + static_assert(sizeof...(Sections) > 0, "SectionSet: a group needs at least one section"); + static_assert(::morph::forms::detail::AllDistinct::value, + "SectionSet: section action types must be pairwise distinct"); + +public: + /// @brief Constructs a section set dispatching through @p handler. + /// @param handler Handler every section dispatches through. Must outlive + /// this `SectionSet`. + /// @param onError Optional callback invoked when a section's dispatch + /// fails. When absent, the error is logged via + /// `morph::log::logError`. Stored and invoked for this + /// object's whole lifetime, so anything the callable refers + /// to must outlive it. + explicit SectionSet(::morph::bridge::BridgeHandler& handler MORPH_LIFETIMEBOUND, + std::function onError MORPH_LIFETIMEBOUND = nullptr) + : _handler{handler}, _onError{std::move(onError)} {} + + SectionSet(const SectionSet&) = delete; + SectionSet& operator=(const SectionSet&) = delete; + SectionSet(SectionSet&&) = delete; + SectionSet& operator=(SectionSet&&) = delete; + + /// @brief Stops every callback this set installed from being delivered. + /// + /// There is nothing to detach: a section's continuations are owned by the + /// in-flight dispatch, not held in a map this object could remove itself + /// from. `_callbacks` gates each one on a token it checks before touching + /// `this`, so a completion resolving after this object is gone finds the + /// token stopped and returns without dereferencing anything. + /// + /// `requestStop()` is called explicitly rather than left to the member's + /// own destruction, even though `_callbacks` is declared last: members are + /// destroyed only *after* the destructor body runs, so a body that later + /// grew a call pumping an event loop would otherwise deliver into a + /// half-dead object. The body does not do that today; stopping first keeps + /// it correct if one is ever added. + /// + /// How strong the gate is depends on which thread destroys this object, + /// exactly as `CallbackScope`'s "Boundary of the guarantee" describes. + /// Destroying it off the delivery thread is advisory only, and that caller + /// owns its own synchronisation. + ~SectionSet() { _callbacks.requestStop(); } + + /// @brief Sets one field of its section's draft, dispatching that section + /// if the draft is now ready. + /// + /// Unlike `FlowSession::set<>` this imposes no ordering: the field's action + /// need only be one of the declared sections. Readiness is evaluated on a + /// copy taken under the lock, so the dispatch happens with the lock + /// released and a concurrent edit to another section cannot block on it. + /// @tparam FieldPtr Pointer-to-data-member of a declared section's action struct. + /// @param value New value for the field. + template + void set(typename ::morph::bridge::detail::MemberPointerTraits::ValueType value) { + using A = typename ::morph::bridge::detail::MemberPointerTraits::ClassType; + static_assert((std::is_same_v || ...), + "SectionSet::set<>: the field's action is not a section of this group"); + A draft{}; + { + std::scoped_lock const lock{_mtx}; + std::get(_drafts).*FieldPtr = std::move(value); + draft = std::get(_drafts); + } + if (::morph::model::ActionValidator::ready(draft)) { + fire(std::move(draft)); + } + } + + /// @brief Clears one section's draft back to a default-constructed action. + /// + /// Touches no other section and dispatches nothing. Values already captured + /// from a previous successful dispatch stay in `resolved()`: they describe + /// what the model was told, which resetting an editor does not undo. + /// @tparam A The section's action type. + template + void reset() { + static_assert((std::is_same_v || ...), + "SectionSet::reset<>: not a section of this group"); + std::scoped_lock const lock{_mtx}; + std::get(_drafts) = A{}; + } + + /// @brief Snapshots one section's current draft. + /// + /// A copy taken under the lock, not a reference into live state, so a + /// renderer can read one section while another thread edits a different one. + /// @tparam A The section's action type. + /// @return The draft as it stands. + template + [[nodiscard]] A draft() const { + static_assert((std::is_same_v || ...), + "SectionSet::draft<>: not a section of this group"); + std::scoped_lock const lock{_mtx}; + return std::get(_drafts); + } + + /// @brief Looks up a value captured from a section's submitted draft or result. + /// @param path `"."`, matching a section's declared `Bind::path()`. + /// @return The field's JSON-encoded value, or `std::nullopt` if @p path was + /// never captured — the section never fired successfully, or has no + /// such field. + [[nodiscard]] std::optional resolved(std::string_view path) const { + std::scoped_lock const lock{_mtx}; + auto iter = _resolvedValues.find(std::string{path}); + if (iter == _resolvedValues.end()) { + return std::nullopt; + } + return iter->second; + } + +private: + /// @brief Records section @p A's submitted draft and its result under + /// `"."` keys. + /// + /// Draft fields go in first and result fields overwrite them on a name + /// collision: the result is what the model actually settled on, so it is + /// the value a later section should prefill from. + /// @tparam A Section action type. + /// @param result The dispatch's successful result. + template + void captureResult(const ::morph::model::ActionTraits::Result& result) { + std::scoped_lock const lock{_mtx}; + auto const typeId = ::morph::model::ActionTraits::typeId(); + auto record = [&](const auto& value) { + ::morph::forms::detail::forEachNamedMember( + value, [&](std::string_view name, const auto& member) { + static_cast(I); + std::string json; + // Mirrors flows.hpp: a field glaze cannot write is skipped + // rather than stored as a broken string. The false arm is + // unreachable for any type that got this far, since the + // same writer already served schema generation. + if (!glz::write_json(member, json)) { + _resolvedValues[std::string{typeId} + "." + std::string{name}] = std::move(json); + } + }); + }; + record(std::get(_drafts)); + record(result); + } + + /// @brief Reports a dispatch failure no `onError` callback was given for. + /// @param typeId The failing section's action type-id. + /// @param err The captured exception. + static void logUnhandledError(std::string_view typeId, const std::exception_ptr& err) { + try { + std::rethrow_exception(err); + } catch (const std::exception& exc) { + ::morph::log::logError(std::string{"[section:"} + std::string{typeId} + + "] unhandled exception: " + exc.what()); + } catch (...) { + ::morph::log::logError(std::string{"[section:"} + std::string{typeId} + "] unhandled unknown exception"); + } + } + + /// @brief Dispatches section @p A's ready draft and routes its outcome. + /// + /// Both closures are gated on `_callbacks`, so neither touches anything on + /// `this` once the set has been destroyed — a completion can still resolve + /// after the set is gone. + /// + /// Nothing here is keyed to a "current" section, which is what makes + /// sections cheaper than flow steps: a late reply cannot be stale, because + /// there is no position for it to be stale relative to. + /// @tparam A Section action type. + /// @param draft The ready action to execute. + template + void fire(A draft) { + _handler.execute(std::move(draft)) + .then(_callbacks, + [this](::morph::model::ActionTraits::Result result) { this->template captureResult(result); }) + .onError(_callbacks, [this](const std::exception_ptr& err) { + if (_onError) { + _onError(err); + } else { + logUnhandledError(::morph::model::ActionTraits::typeId(), err); + } + }); + } + + ::morph::bridge::BridgeHandler& _handler; + std::function _onError; + // _handler/_onError are set once at construction and never reassigned. + // Everything below is touched both by the owning thread (set/reset/draft) + // and by a dispatch's continuation, which runs on whatever thread resolves + // the BridgeHandler completion -- see docs/spec/core/bridge.md's + // executor/callback model. + mutable std::mutex _mtx; + std::tuple _drafts{}; + std::unordered_map _resolvedValues; + // Declared last, so it is the first member destroyed: every gated callback + // is stopped before the state those callbacks touch goes away. + ::morph::async::CallbackScope _callbacks; +}; + } // namespace morph::forms // clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". diff --git a/tests/test_sections.cpp b/tests/test_sections.cpp index 4b11d080..1c3538e3 100644 --- a/tests/test_sections.cpp +++ b/tests/test_sections.cpp @@ -128,3 +128,69 @@ TEST_CASE("sectionGroupSchemaJson carries each section's title, action and binds CHECK_FALSE(sections[0].contains("index")); CHECK_FALSE(sections[1].contains("index")); } + +// --------------------------------------------------------------------------- +// Each case builds its own bridge/handler/section set: a SectionSet holds no +// global state, and a shared one would let a late dispatch from a previous case +// land in this one's recorder. +// --------------------------------------------------------------------------- + +TEST_CASE("SectionSet: sections fire independently, in any order", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + + // Edit the SECOND section first. Under FlowSession this throws + // std::logic_error -- "field belongs to an action that is not the current + // step" -- which is exactly the gap morph#513 reports. + sections.set<&SecPrefs::theme>("dark"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + + // Then the first. Both fire; neither was ever "current". + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); + + auto const fired = recorder().snapshot(); + CHECK(fired[0] == "SectionsTest_Prefs"); + CHECK(fired[1] == "SectionsTest_Profile"); +} + +TEST_CASE("SectionSet: a not-ready draft does not dispatch", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + + // SecPrefs::validate() requires a non-empty theme; profileId alone is not + // ready. Without the readiness gate this would dispatch a half-filled action. + sections.set<&SecPrefs::profileId>(7); + CHECK(recorder().snapshot().empty()); + + // Completing it dispatches, carrying the field set earlier. + sections.set<&SecPrefs::theme>("light"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); +} + +TEST_CASE("SectionSet: an already-fired section fires again on the next edit", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + + // No latch: the draft is still ready, so it dispatches again. Matching + // FlowSession, which also re-fires a ready step on every set<>. + sections.set<&SecProfile::name>("grace"); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); +} From 3950cef8b190de7834e6357c5ef47cbde6ab1089 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:50:19 +0200 Subject: [PATCH 07/13] forms: cover SectionSet's draft, capture, error and lifetime behaviour Nine cases, each verified to fail with its implementation removed except where noted below. Two of them measured nothing as first written, and the mutation run is what said so: - "a not-ready draft" checked the recorder immediately after an incomplete set<>. Dispatch is asynchronous, so the check passed whether or not the gate existed. It now watches onError instead, which is where a missing gate is actually visible: the bridge enforces ActionValidator on its own path (bridge.hpp), so an ungated draft never reaches execute() either -- it comes back as a validation failure. The round trip that can only fail is the cost the gate avoids. - the lifetime case destroyed the set with nothing genuinely in flight, so it exercised no window at all. It now uses a section whose model call blocks until the test releases it, released only after the scope closes. That case still cannot distinguish the destructor's explicit requestStop() from CallbackScope's own destructor -- both stop delivery, and the window a member-reordering regression would open is a few instructions wide. It guards the property that matters (a completion arriving after destruction delivers nothing); the explicit call stays as forward-defence for a destructor body that might one day pump, as its comment says. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- tests/test_sections.cpp | 176 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 169 insertions(+), 7 deletions(-) diff --git a/tests/test_sections.cpp b/tests/test_sections.cpp index 1c3538e3..1f26fd79 100644 --- a/tests/test_sections.cpp +++ b/tests/test_sections.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -14,6 +15,7 @@ #include #include #include +#include #include #include "test_support.hpp" @@ -74,6 +76,30 @@ struct SecExplodesResult { std::int64_t id = 0; }; +// A section whose model call blocks until the test releases it, so a dispatch +// can be held genuinely in flight across the SectionSet's destruction. Both +// flags are function-local statics for the same reason as the recorder: the +// model's execute() and the test body both need them, and SecModel is +// stateless. Mirrors test_flows_apps.cpp's FlowStepSlow. +namespace { +std::atomic& secSlowStarted() { + static std::atomic flag{false}; + return flag; +} +std::atomic& secSlowRelease() { + static std::atomic flag{false}; + return flag; +} +} // namespace + +struct SecSlow { + std::string label; + [[nodiscard]] bool validate() const { return !label.empty(); } +}; +struct SecSlowResult { + std::int64_t id = 0; +}; + struct SecModel { SecProfileResult execute(SecProfile action) { recorder().record("SectionsTest_Profile"); @@ -87,17 +113,26 @@ struct SecModel { recorder().record("SectionsTest_Explodes"); throw std::runtime_error{"section boom"}; } + static SecSlowResult execute(const SecSlow& /*action*/) { + secSlowStarted().store(true, std::memory_order_relaxed); + while (!secSlowRelease().load(std::memory_order_relaxed)) { + std::this_thread::sleep_for(std::chrono::milliseconds{1}); + } + throw std::runtime_error{"late section boom"}; + } }; BRIDGE_REGISTER_MODEL(SecModel, "SectionsTest_Model") BRIDGE_REGISTER_ACTION(SecModel, SecProfile, "SectionsTest_Profile") BRIDGE_REGISTER_ACTION(SecModel, SecPrefs, "SectionsTest_Prefs") BRIDGE_REGISTER_ACTION(SecModel, SecExplodes, "SectionsTest_Explodes") +BRIDGE_REGISTER_ACTION(SecModel, SecSlow, "SectionsTest_Slow") using ProfileSection = morph::forms::Section; using PrefsSection = morph::forms::Section>; using ExplodesSection = morph::forms::Section; +using SlowSection = morph::forms::Section; using DemoGroup = morph::forms::SectionGroup<"Account settings", ProfileSection, PrefsSection>; BRIDGE_REGISTER_SECTION_GROUP(DemoGroup, "SectionsTest_DemoGroup") @@ -159,23 +194,37 @@ TEST_CASE("SectionSet: sections fire independently, in any order", "[sections]") CHECK(fired[1] == "SectionsTest_Profile"); } -TEST_CASE("SectionSet: a not-ready draft does not dispatch", "[sections]") { +TEST_CASE("SectionSet: a not-ready draft is not sent at all", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; morph::testing::InlineExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; - morph::forms::SectionSet sections{handler}; + + // The gate is observed through onError, not through the recorder. The + // bridge enforces ActionValidator on its own dispatch path (bridge.hpp), + // so an ungated draft would never reach SecModel::execute either -- it + // would come back as a validation failure. A round trip that can only fail + // is the cost this gate exists to avoid, and the error callback is where + // that cost is visible. + std::atomic errors{0}; + morph::forms::SectionSet sections{ + handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; recorder().clear(); - // SecPrefs::validate() requires a non-empty theme; profileId alone is not - // ready. Without the readiness gate this would dispatch a half-filled action. + // SecPrefs::validate() requires a non-empty theme; profileId alone is not ready. sections.set<&SecPrefs::profileId>(7); - CHECK(recorder().snapshot().empty()); + CHECK(morph::testing::waitUntil([&errors] { return errors.load() != 0; }, std::chrono::milliseconds{300}) == + false); - // Completing it dispatches, carrying the field set earlier. + // Completing it dispatches, carrying the field set earlier, and succeeds. + // Waiting on the capture rather than on the recorder: the recorder is + // written inside execute(), which runs before the completion that captures. sections.set<&SecPrefs::theme>("light"); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Prefs.summary").has_value(); })); + CHECK(recorder().snapshot().size() == 1); + CHECK(errors.load() == 0); + CHECK(sections.resolved("SectionsTest_Prefs.profileId") == std::string{"7"}); } TEST_CASE("SectionSet: an already-fired section fires again on the next edit", "[sections]") { @@ -194,3 +243,116 @@ TEST_CASE("SectionSet: an already-fired section fires again on the next edit", " sections.set<&SecProfile::name>("grace"); REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); } + +TEST_CASE("SectionSet: reset clears one section and leaves the others intact", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + sections.set<&SecPrefs::profileId>(42); // not ready: no theme yet + sections.set<&SecProfile::name>("ada"); // ready: fires + + CHECK(sections.draft().profileId == 42); + CHECK(sections.draft().name == "ada"); + + sections.reset(); + + CHECK(sections.draft().profileId == 0); + // Per-section isolation is the point: resetting one editor must not wipe + // what the user typed into another. + CHECK(sections.draft().name == "ada"); +} + +TEST_CASE("SectionSet: a fired section's fields are resolvable; an unfired one is not", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + + // The result's field: SecProfileResult::id == name.size() == 3. + CHECK(sections.resolved("SectionsTest_Profile.id") == std::string{"3"}); + // The submitted draft's field is captured too, JSON-encoded. + CHECK(sections.resolved("SectionsTest_Profile.name") == std::string{R"("ada")"}); + + // Both halves matter. Without these an implementation that returned a value + // for every path would still pass the checks above. + CHECK_FALSE(sections.resolved("SectionsTest_Prefs.summary").has_value()); + CHECK_FALSE(sections.resolved("SectionsTest_Profile.nosuchfield").has_value()); +} + +TEST_CASE("SectionSet: a failing dispatch reaches the onError callback", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic errors{0}; + morph::forms::SectionSet sections{ + handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; + + recorder().clear(); + sections.set<&SecExplodes::label>("boom"); + REQUIRE(morph::testing::waitUntil([&] { return errors.load() == 1; })); + + // A section that succeeds does not route to onError, so the count above is + // the failure and not merely "some callback ran". + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + CHECK(errors.load() == 1); +} + +TEST_CASE("SectionSet: an unhandled failure logs instead of escaping", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + // No onError: the failure has nowhere to go but the log. It must not + // propagate out of the completion and take the executor thread down. + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + REQUIRE_NOTHROW(sections.set<&SecExplodes::label>("boom")); + REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + + // The set survives its own failed section: an unrelated one still works. + sections.set<&SecProfile::name>("ada"); + REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); +} + +TEST_CASE("SectionSet: destroying it with a dispatch in flight delivers nothing", "[sections]") { + morph::exec::ThreadPoolExecutor pool{2}; + morph::testing::InlineExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(pool)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + secSlowStarted().store(false); + secSlowRelease().store(false); + + std::atomic errors{0}; + { + morph::forms::SectionSet sections{ + handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; + sections.set<&SecSlow::label>("held"); + // Wait until the model call is genuinely inside execute() before + // leaving the scope. Without this the dispatch would usually finish + // first and the destructor would race nothing at all. + REQUIRE(morph::testing::waitUntil([] { return secSlowStarted().load(); })); + } + + // The set is gone; only now does the model return (by throwing). Its error + // continuation resolves against an object that no longer exists, and must + // find the callback scope stopped and do nothing. + secSlowRelease().store(true); + CHECK(morph::testing::waitUntil([&errors] { return errors.load() != 0; }, std::chrono::milliseconds{500}) == + false); + CHECK(errors.load() == 0); +} From 76f79f12fc0b6885f033afb835c03b87f892fbc6 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 21:54:51 +0200 Subject: [PATCH 08/13] docs: spec for unordered sections (#513) Also fixes a dangling section citation the prose lint caught in the design doc: callback_scope.md's heading is "Thread safety and the boundary of the guarantee", not "Boundary of the guarantee". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/forms/sections.md | 288 ++++++++++++++++++ .../plans/2026-09-10-unordered-sections.md | 3 +- .../2026-09-10-unordered-sections-design.md | 2 +- 3 files changed, 291 insertions(+), 2 deletions(-) create mode 100644 docs/spec/forms/sections.md diff --git a/docs/spec/forms/sections.md b/docs/spec/forms/sections.md new file mode 100644 index 00000000..224577db --- /dev/null +++ b/docs/spec/forms/sections.md @@ -0,0 +1,288 @@ +# `morph::forms::SectionSet` — unordered sections + +`SectionSet` drives N independently editable action drafts +on one screen. Each section accumulates its own draft through `set<>` and +dispatches through `BridgeHandler::execute()` as soon as its own +`ActionValidator::ready` accepts it. There is no active section, no index, +and no sequence. Like [workflows_navigation.md](workflows_navigation.md)'s +wizards, this is additive metadata and client-side bookkeeping over the +dispatch path in [../core/bridge.md](../core/bridge.md) — no new wire format, +no new execution mode. + +## Contents + +- [The gap this closes](#the-gap-this-closes) +- [The `s-*` section-group document](#the-s--section-group-document) +- [C++ descriptors](#c-descriptors) +- [`SectionSet`](#sectionsetmodel-sections) +- [Prefill is a declaration, not a write](#prefill-is-a-declaration-not-a-write) +- [Concurrency and lifetime](#concurrency-and-lifetime) +- [Compile-time contract](#compile-time-contract) +- [Design decisions](#design-decisions) +- [Limitations](#limitations) +- [Testing](#testing) +- [Cross-references](#cross-references) + +## The gap this closes + +`FlowSession` has exactly one current step, and `FlowSession::set<>` throws +`std::logic_error` on a field belonging to any other. That is right for a +wizard and wrong for a screen whose blocks have no order — a settings page, a +tab strip, a column of cards — where a user may edit the third block first and +never touch the second. Before this layer such a screen either hand-wired one +`BridgeHandler::execute` call site per block, re-implementing draft +accumulation and the readiness gate each time, or misused a wizard and got a +`logic_error` for editing its own form out of order (morph#513). + +`SectionSet` keeps everything `FlowSession` does per action — per-action draft +accumulation, the readiness gate, result capture, error routing, the callback +lifetime gate — and drops only the position. + +Choose `FlowSession` when order carries meaning (a later step needs an earlier +step's result, or the user must not skip ahead) and `SectionSet` when it does +not. + +## The `s-*` section-group document + +`sectionGroupSchemaJson()` emits a small JSON document alongside each +section's ordinary action schema ([forms.md](forms.md)): + +```json +{ + "s-id": "AccountSettings", + "s-title": "Account settings", + "s-sections": [ + { "action": "UpdateProfile", "title": "Profile" }, + { "action": "UpdatePrefs", "title": "Preferences", + "prefill": { "profileId": "UpdateProfile.id" } } + ] +} +``` + +| Key | Where | JSON type | Meaning | +|---|---|---|---| +| `s-id` | top-level | string | The group's registered type-id (`SectionGroupTraits::typeId()`). | +| `s-title` | top-level | string | Human title for the whole group. | +| `s-sections` | top-level | array | The group's sections. Array order is declaration order and carries no meaning. | +| ↳ `action` | section | string | The section's registered action type-id (`ActionTraits::typeId()`). | +| ↳ `title` | section | string | Human title for the section (tab label, card header). | +| ↳ `prefill` | section | object | Field name → `"."` source path. Present only when the section declares at least one `Bind`. | + +There is deliberately **no index or order key**. A renderer arranges the +sections itself — as tabs, as a grid, collapsed, in a user-chosen order — and +a position in the wire format would suggest a sequence a section group does not +have. That is the one thing distinguishing this document from `w-*`, so +emitting an index would erase the distinction the type exists to make. + +`s-sections` is a JSON array only because JSON has no unordered collection that +also preserves duplicates-free keys usefully; consumers must not read meaning +into the order. + +## C++ descriptors + +```cpp +struct UpdateProfile { std::string name; bool validate() const { return !name.empty(); } }; +struct UpdatePrefs { std::int64_t profileId; std::string theme; + bool validate() const { return !theme.empty(); } }; + +BRIDGE_REGISTER_ACTION(SettingsModel, UpdateProfile, "UpdateProfile") +BRIDGE_REGISTER_ACTION(SettingsModel, UpdatePrefs, "UpdatePrefs") + +using ProfileSection = morph::forms::Section; +using PrefsSection = morph::forms::Section>; + +using AccountSettings = morph::forms::SectionGroup<"Account settings", + ProfileSection, PrefsSection>; +BRIDGE_REGISTER_SECTION_GROUP(AccountSettings, "AccountSettings") +``` + +| Type | Members | Meaning | +|---|---|---| +| `Section` | `action`, `binds`, `title()` | One section: a registered action, a display title, zero or more prefill declarations. | +| `SectionGroup` | `sections`, `title()` | A group of sections sharing one screen. | +| `SectionGroupTraits` | `typeId()` | Maps a group type to its stable string id. Specialise via `BRIDGE_REGISTER_SECTION_GROUP`; the default is a forward declaration, so using it unregistered is an incomplete-type error. | +| `Bind` | `field()`, `path()` | Shared with `morph::flows` — see [Prefill is a declaration, not a write](#prefill-is-a-declaration-not-a-write). | + +`Bind` and the declaration-walking helpers live in +`morph/forms/detail/session_common.hpp`, shared by both session types. +`morph::flows::Bind` remains as an alias, since that is the name existing +consumers write. + +Registration is metadata only. `BRIDGE_REGISTER_SECTION_GROUP` specialises a +traits template and registers nothing with the dispatcher — exactly as +`BRIDGE_REGISTER_WIZARD` does, and for the same reason ([../core/registry.md](../core/registry.md)). + +## `SectionSet` + +```cpp +morph::forms::SectionSet sections{handler}; + +sections.set<&UpdatePrefs::theme>("dark"); // fires UpdatePrefs -- no ordering +sections.set<&UpdateProfile::name>("ada"); // fires UpdateProfile +``` + +| Member | Contract | +|---|---| +| `SectionSet(handler, onError = nullptr)` | `handler` must outlive the set. `onError` receives every failed dispatch; when absent, failures are logged via `morph::log::logError` and never escape the completion. | +| `set(value)` | Assigns one field of its section's draft, then dispatches that section if `ActionValidator::ready` now accepts the draft. The field's action need only be *one of* the declared sections — there is no current one. | +| `reset()` | Clears section `A`'s draft to a default-constructed action. Touches no other section and dispatches nothing. Values already in `resolved()` stay: they describe what the model was told, which resetting an editor does not undo. | +| `draft()` | Returns a copy of section `A`'s draft, taken under the lock. | +| `resolved(path)` | Returns the JSON-encoded value captured at `"."`, or `std::nullopt` when that path was never captured. | + +`SectionSet` is neither copyable nor movable: its callbacks capture `this`. + +**No latch.** A ready section re-fires on *every* subsequent `set<>`, matching +`FlowSession`. A caller wanting one request per pause debounces on its own +side, where it knows what a pause means for its input widget. Coalescing here +would have to guess. + +**A ready draft is dispatched, a not-ready one is not sent at all.** The gate +is not a correctness backstop — `BridgeHandler::execute` enforces +`ActionValidator` on its own path ([../core/bridge.md](../core/bridge.md)), so +an ungated draft would come back as a validation failure rather than execute. +The gate exists to avoid a round trip that can only fail, and its absence is +observable as spurious `onError` calls. + +## Prefill is a declaration, not a write + +A `Bind` on a section says *where a field's initial value comes from*. Nothing +in the framework assigns it. `sectionGroupSchemaJson` emits it under `prefill` +for a renderer to act on, and `resolved(path)` exposes the captured values a +renderer resolves it against. `SectionSet` never writes a bound field into a +draft on its own. + +This is exact parity with `FlowSession`, which also emits `prefill` metadata +and captures values without ever assigning a bound field. A section set had a +stronger temptation to differ — with no ordering, "fill in the dependent field +the moment the source resolves" is a coherent design — but a reactive write +would silently overwrite a value the user had already typed into that field, +with no signal that it happened, and only for bound fields. The renderer knows +whether its widget is dirty; the session does not. + +Capture happens on success only, in the dispatch's completion: + +- the submitted draft's fields are recorded first, then the result's fields, + so a result field wins on a name collision — the result is what the model + actually settled on, and is therefore what a dependent field should show; +- keys are `"."`, the same vocabulary `Bind::path()` uses; +- values are JSON-encoded, so `resolved("Profile.name")` yields `"\"ada\""` + and `resolved("Profile.id")` yields `"3"`. + +A failed dispatch captures nothing. + +## Concurrency and lifetime + +One mutex guards the drafts and the captured values. `set<>` takes it to +assign the field and snapshot the draft, then **releases it before +dispatching**, so a slow dispatch of one section cannot block an edit to +another. + +Dispatch continuations run on whatever executor resolves the underlying +`BridgeHandler` completion, not necessarily the thread that called `set<>` — +see [../core/bridge.md](../core/bridge.md)'s executor/callback model. + +Every continuation is gated on one `morph::async::CallbackScope` +([../core/callback_scope.md](../core/callback_scope.md)), declared last so it +is the first member destroyed, and stopped explicitly at the top of +`~SectionSet`. A completion resolving after the set is gone finds the token +stopped and returns without touching anything. As always, how strong that +guarantee is depends on which thread destroys the set — destroying it off the +delivery thread is advisory, and that caller owns its own synchronisation. + +Unlike `FlowSession`, nothing here is keyed to a current position, so a reply +arriving late cannot be *stale*: there is no position for it to be stale +relative to. That is why `SectionSet` needs no equivalent of `FlowSession`'s +`_activeStep` guard. + +## Compile-time contract + +Two `static_assert`s, both on `SectionSet`: + +```cpp +// Rejected: a group must have at least one section. +morph::forms::SectionSet empty{handler}; + +// Rejected: two sections of the same action would share one draft slot, +// so an edit to either would silently clobber the other. +morph::forms::SectionSet duplicate{handler}; +``` + +and one on each of `set<>`, `reset()` and `draft()`: + +```cpp +// Rejected: UnrelatedAction is not a section of this group. +sections.set<&UnrelatedAction::field>(1); +``` + +This is where `SectionSet` still refuses a field — but at compile time, on +membership, not at run time on position. `FlowSession`'s `std::logic_error` +has no counterpart: with no current step, there is no run-time state that can +make a member field wrong to set. + +## Design decisions + +- **A separate type rather than a `FlowSession` mode.** A flag ("unordered + flow") would leave `advance()`, `back()`, `currentIndex()` and `finished()` + on an object for which none of them mean anything, and every one of them + would need a documented answer for the unordered case. A separate type has + only the members that make sense. +- **Shared declaration vocabulary.** `Bind`, the tuple/pack walkers and the + distinctness trait moved to `forms/detail/session_common.hpp` rather than + being duplicated. They describe how a form session declares its units, which + both types do identically; only the sequencing differs. +- **No index in the schema.** See [The `s-*` section-group document](#the-s--section-group-document). +- **No aggregate readiness and no "submit all".** Sections are independent by + construction; a group-level submit would reintroduce a coordination point + and raise questions this layer has no answer for (partial failure, ordering, + atomicity). Cross-action atomicity belongs in the outbox + ([../journal/journal.md](../journal/journal.md)), as it does for wizards. + +## Limitations + +- No renderer ships for `s-*` yet. `WizardView.qml` has no section-group + counterpart in `src/qt/forms`; a host consuming the document builds its own + layout for now. +- `resolved()` returns JSON-encoded strings, not typed values — the same shape + `FlowSession::resolved` has, and the same caller-side decode. +- A section that fires repeatedly issues one dispatch per `set<>`; there is no + in-flight coalescing or cancellation of a superseded request. +- Prefill is never applied by the framework, so a host that ignores the + `prefill` metadata gets no prefilling at all. + +## Testing + +`tests/test_sections.cpp` (`[sections]`), nine cases: + +- `sectionGroupSchemaJson` emits `s-id`, `s-title`, each section's `action` + and `title`, `prefill` only where a `Bind` is declared, and no `index` key. +- Sections fire independently in any order — the morph#513 regression: the + same edit sequence throws `std::logic_error` under `FlowSession`. +- A not-ready draft is not sent at all, observed through `onError` (a missing + gate is visible as a spurious validation failure, not as a bad execution). +- An already-fired section fires again on the next edit (the no-latch rule). +- `reset()` clears one section and leaves the others intact. +- A fired section's draft *and* result fields are resolvable, and a path + belonging to an unfired section — or to no field — is not. +- A failing dispatch reaches `onError`, and a succeeding one does not. +- An unhandled failure logs instead of escaping, and the set survives it. +- Destroying the set with a dispatch genuinely in flight (a section whose + model call blocks until the test releases it) delivers nothing afterwards. + +## Cross-references + +- [workflows_navigation.md](workflows_navigation.md) — `FlowSession` and the + `w-*` document this layer is the unordered sibling of; the ordering + constraint whose absence defines `SectionSet`. +- [forms.md](forms.md) — the per-action schema each section renders, and + `FixedString`, which `Section`/`SectionGroup` titles and `Bind` paths use. +- [../core/bridge.md](../core/bridge.md) — `BridgeHandler::execute` and + `ActionValidator::ready`, the dispatch path and readiness gate `SectionSet` + reuses without extending. +- [../core/callback_scope.md](../core/callback_scope.md) — + `morph::async::CallbackScope`, the gate that refuses a completion resolving + after the set is destroyed. +- [../core/registry.md](../core/registry.md) — `ActionTraits::typeId()` and the + metadata-only registration `BRIDGE_REGISTER_SECTION_GROUP` mirrors. +- [../journal/journal.md](../journal/journal.md) — where cross-action + atomicity belongs; a section group deliberately does not provide it. diff --git a/docs/superpowers/plans/2026-09-10-unordered-sections.md b/docs/superpowers/plans/2026-09-10-unordered-sections.md index 1dd40c2f..f489eae4 100644 --- a/docs/superpowers/plans/2026-09-10-unordered-sections.md +++ b/docs/superpowers/plans/2026-09-10-unordered-sections.md @@ -1025,7 +1025,8 @@ the destructor anyway per Step 3 and keep the case as the regression guard for a /// if one is added. /// /// The strength of the gate depends on which thread destroys this object, - /// exactly as `CallbackScope`'s "Boundary of the guarantee" describes. + /// exactly as callback_scope.md's "Thread safety and the boundary of + /// the guarantee" describes. /// Destroying it off the delivery thread is advisory only, and that caller /// owns its own synchronisation. ~SectionSet() { _callbacks.requestStop(); } diff --git a/docs/superpowers/specs/2026-09-10-unordered-sections-design.md b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md index a7d7c509..3ed9c104 100644 --- a/docs/superpowers/specs/2026-09-10-unordered-sections-design.md +++ b/docs/superpowers/specs/2026-09-10-unordered-sections-design.md @@ -160,7 +160,7 @@ The guarantee has the same boundary as `FlowSession`'s and the same caveat applies verbatim: destroying the session on the thread its continuations are delivered on makes check-then-run atomic; destroying it from another thread is advisory, and that caller owns its own synchronisation. See -`docs/spec/core/callback_scope.md`, "Boundary of the guarantee". +`docs/spec/core/callback_scope.md`, "Thread safety and the boundary of the guarantee". ## Schema document From bcc23345c360dab38ba952933a14358b5cf3a8ad Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 22:02:15 +0200 Subject: [PATCH 09/13] forms: clear clang-tidy on every changed line CI's clang-tidy-diff analyses changed lines in headers too, and moving the session helpers into a new file makes every one of their lines changed. Real fixes where there was one (redundant typename, a result copied per invocation, three test model params, three exception_ptr lambdas); the repo's established NOLINT markers for the two that have no fix -- a visitor invoked once per element must not be moved from, and a registration macro is the intended public API (forms.hpp, views.hpp, registry.hpp all say so). The NOLINTNEXTLINE reasons are on the marker's own line: a reason wrapped onto a second comment line makes the *comment* the next line, and the suppression silently misses. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/forms/detail/session_common.hpp | 3 +++ include/morph/forms/sections.hpp | 10 +++++++--- tests/test_sections.cpp | 12 ++++++------ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/include/morph/forms/detail/session_common.hpp b/include/morph/forms/detail/session_common.hpp index 940187de..07ab6e06 100644 --- a/include/morph/forms/detail/session_common.hpp +++ b/include/morph/forms/detail/session_common.hpp @@ -51,7 +51,9 @@ namespace detail { /// @tparam Visitor Callable with a `template operator()()`. /// @param visitor Callable invoked once per tuple element. template +// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) — invoked once per element, never moved from constexpr void forEachTupleElement(Visitor&& visitor) { + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) — same: one call per element [](std::index_sequence, Visitor&& innerVisitor) { (innerVisitor.template operator(), I>(), ...); }(std::make_index_sequence>{}, std::forward(visitor)); @@ -65,6 +67,7 @@ constexpr void forEachTupleElement(Visitor&& visitor) { /// @param index 0-based position to visit. /// @param visitor Callable invoked for the element at @p index. template +// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) — the visited element is a run-time choice constexpr void forPackElement(std::size_t index, Visitor&& visitor) { std::size_t i = 0; (void)((i++ == index ? (visitor.template operator()(), true) : false) || ...); diff --git a/include/morph/forms/sections.hpp b/include/morph/forms/sections.hpp index cb28a3a9..34435886 100644 --- a/include/morph/forms/sections.hpp +++ b/include/morph/forms/sections.hpp @@ -183,8 +183,8 @@ class SectionSet { /// @tparam FieldPtr Pointer-to-data-member of a declared section's action struct. /// @param value New value for the field. template - void set(typename ::morph::bridge::detail::MemberPointerTraits::ValueType value) { - using A = typename ::morph::bridge::detail::MemberPointerTraits::ClassType; + void set(::morph::bridge::detail::MemberPointerTraits::ValueType value) { + using A = ::morph::bridge::detail::MemberPointerTraits::ClassType; static_assert((std::is_same_v || ...), "SectionSet::set<>: the field's action is not a section of this group"); A draft{}; @@ -300,7 +300,9 @@ class SectionSet { void fire(A draft) { _handler.execute(std::move(draft)) .then(_callbacks, - [this](::morph::model::ActionTraits::Result result) { this->template captureResult(result); }) + [this](const ::morph::model::ActionTraits::Result& result) { + this->template captureResult(result); + }) .onError(_callbacks, [this](const std::exception_ptr& err) { if (_onError) { _onError(err); @@ -328,6 +330,7 @@ class SectionSet { } // namespace morph::forms // clang-format off -- public macro surface; see CONTRIBUTING.md, "Formatting/linting". +// NOLINTBEGIN(cppcoreguidelines-macro-usage) — registration macro is the intended public API /// @brief Specialises `morph::forms::SectionGroupTraits` with the string type-id @p NAME. #define BRIDGE_REGISTER_SECTION_GROUP(G, NAME) \ template <> \ @@ -335,3 +338,4 @@ class SectionSet { static constexpr std::string_view typeId() noexcept { return NAME; } \ }; // clang-format on +// NOLINTEND(cppcoreguidelines-macro-usage) diff --git a/tests/test_sections.cpp b/tests/test_sections.cpp index 1f26fd79..f6cc94a0 100644 --- a/tests/test_sections.cpp +++ b/tests/test_sections.cpp @@ -101,15 +101,15 @@ struct SecSlowResult { }; struct SecModel { - SecProfileResult execute(SecProfile action) { + SecProfileResult execute(const SecProfile& action) { recorder().record("SectionsTest_Profile"); return {.id = static_cast(action.name.size())}; } - SecPrefsResult execute(SecPrefs action) { + SecPrefsResult execute(const SecPrefs& action) { recorder().record("SectionsTest_Prefs"); return {.summary = action.theme}; } - SecExplodesResult execute(SecExplodes) { + SecExplodesResult execute(const SecExplodes&) { recorder().record("SectionsTest_Explodes"); throw std::runtime_error{"section boom"}; } @@ -208,7 +208,7 @@ TEST_CASE("SectionSet: a not-ready draft is not sent at all", "[sections]") { // that cost is visible. std::atomic errors{0}; morph::forms::SectionSet sections{ - handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; + handler, [&errors](const std::exception_ptr&) { errors.fetch_add(1); }}; recorder().clear(); @@ -296,7 +296,7 @@ TEST_CASE("SectionSet: a failing dispatch reaches the onError callback", "[secti std::atomic errors{0}; morph::forms::SectionSet sections{ - handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; + handler, [&errors](const std::exception_ptr&) { errors.fetch_add(1); }}; recorder().clear(); sections.set<&SecExplodes::label>("boom"); @@ -340,7 +340,7 @@ TEST_CASE("SectionSet: destroying it with a dispatch in flight delivers nothing" std::atomic errors{0}; { morph::forms::SectionSet sections{ - handler, [&errors](std::exception_ptr) { errors.fetch_add(1); }}; + handler, [&errors](const std::exception_ptr&) { errors.fetch_add(1); }}; sections.set<&SecSlow::label>("held"); // Wait until the model call is genuinely inside execute() before // leaving the scope. Without this the dispatch would usually finish From 1a3ce71fd2490bbe887fa40a3f0d90e5d88b33cc Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 22:32:41 +0200 Subject: [PATCH 10/13] forms: fix the teardown race the Windows CI segfault exposed The morph#513 regression test segfaulted on Windows / cl-debug. Not a flake and not a Windows quirk -- a genuine use-after-free my tests caused, and TSan reproduces it about once in fifteen runs locally. CallbackScope::requestStop() does not wait for a callback that is already past its token check; that is its documented contract, and FlowSession has the same property. So a test may only destroy a SectionSet once no completion is running. Mine could not tell: - the recorder is written inside execute(), which runs *before* the completion that touches the SectionSet, so waiting on it proved nothing; - resolved() is no better. captureResult publishes its first key while still writing the rest, so a test that waits for one key and then leaves scope tears the set down underneath its own callback. That is the actual use-after-free TSan reports, in captureResult's write loop. Every case except the lifetime one now uses StepExecutor and an explicit drain(), so the thread that delivers the completion is the thread that destroys the set -- the case CallbackScope's guarantee actually covers. That removes the race rather than narrowing the window, and the tests get to assert exact counts instead of polling for them. 0 failures in 100 TSan runs and 60 ASan runs, from ~1 in 15 before. The lifetime case keeps InlineExecutor deliberately: it exists to destroy the set with a dispatch outstanding, and its blocking action guarantees the completion starts only after the destructor has finished. Also clears the glaze-DOM operator[] findings CI's clang-tidy reported on the changed schema-emitter lines, with the marker forms.hpp already uses. My earlier local run missed them -- filtering clang-tidy's output by filename hid findings the default run suppresses as non-user code; only an explicit --checks run surfaced them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/forms/detail/session_common.hpp | 2 + include/morph/forms/flows.hpp | 1 + include/morph/forms/sections.hpp | 2 + tests/test_sections.cpp | 109 +++++++++++++----- 4 files changed, 87 insertions(+), 27 deletions(-) diff --git a/include/morph/forms/detail/session_common.hpp b/include/morph/forms/detail/session_common.hpp index 07ab6e06..9c4577a5 100644 --- a/include/morph/forms/detail/session_common.hpp +++ b/include/morph/forms/detail/session_common.hpp @@ -91,6 +91,7 @@ struct AllDistinct : std::bool_constant<(!std::is_same_v && /// materialising an empty `prefill` object. /// @tparam BindsTuple `std::tuple...>`. /// @param node Destination JSON object node. +// NOLINTBEGIN(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) -- glaze DOM requires operator[] template void emitBindsInto(glz::generic_u64& node) { forEachTupleElement([&]() { @@ -98,6 +99,7 @@ void emitBindsInto(glz::generic_u64& node) { node[std::string{BindT::field()}] = std::string{BindT::path()}; }); } +// NOLINTEND(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) } // namespace detail } // namespace morph::forms diff --git a/include/morph/forms/flows.hpp b/include/morph/forms/flows.hpp index 2c4c0032..bf559ab6 100644 --- a/include/morph/forms/flows.hpp +++ b/include/morph/forms/flows.hpp @@ -118,6 +118,7 @@ template step["action"] = std::string{::morph::model::ActionTraits::typeId()}; step["title"] = std::string{StepT::title()}; if constexpr (std::tuple_size_v != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) -- glaze DOM requires operator[] ::morph::forms::detail::emitBindsInto(step["prefill"]); } steps.emplace_back(std::move(step)); diff --git a/include/morph/forms/sections.hpp b/include/morph/forms/sections.hpp index 34435886..acd80335 100644 --- a/include/morph/forms/sections.hpp +++ b/include/morph/forms/sections.hpp @@ -92,6 +92,7 @@ struct SectionGroupTraits; // forward — specialise or use BRIDGE_REGISTER_SEC /// @tparam G Concrete `SectionGroup` type. /// @return The group's JSON document. Empty string only if glaze's own JSON /// writer fails on the assembled DOM (schema generation never throws). +// NOLINTBEGIN(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) -- glaze DOM requires operator[] template [[nodiscard]] std::string sectionGroupSchemaJson() { glz::generic_u64 dom{}; @@ -113,6 +114,7 @@ template return glz::write_json(dom).value_or(std::string{}); } +// NOLINTEND(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) /// @brief Drives N independently editable action drafts on one screen. /// diff --git a/tests/test_sections.cpp b/tests/test_sections.cpp index f6cc94a0..bdbbf97b 100644 --- a/tests/test_sections.cpp +++ b/tests/test_sections.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -137,6 +138,31 @@ using SlowSection = morph::forms::Section; using DemoGroup = morph::forms::SectionGroup<"Account settings", ProfileSection, PrefsSection>; BRIDGE_REGISTER_SECTION_GROUP(DemoGroup, "SectionsTest_DemoGroup") +// Delivers every queued completion on the *test* thread and returns how many ran. +// +// SectionSet's callbacks are gated on a CallbackScope, which by contract does +// not wait for a callback already past its token check (callback_scope.md). +// Polling a published value is not enough to know one has finished: captureResult +// makes its first key visible through resolved() while it is still writing the +// rest, so a test that waits on resolved() and then leaves scope can destroy the +// set underneath its own completion. That is a real use-after-free -- it +// segfaulted on Windows CI and TSan reproduces it about once in fifteen runs. +// +// Draining on the test thread removes the race rather than narrowing it: the +// thread that delivers the callback is the thread that destroys the set, which +// is exactly the case CallbackScope's guarantee covers. +namespace { +std::size_t drain(morph::testing::StepExecutor& exec) { + REQUIRE(morph::testing::waitUntil([&exec] { return exec.pending() > 0; })); + std::size_t ran = exec.runAll(); + // A completion can post follow-up work; keep going while more arrives. + while (morph::testing::waitUntil([&exec] { return exec.pending() > 0; }, std::chrono::milliseconds{50})) { + ran += exec.runAll(); + } + return ran; +} +} // namespace + TEST_CASE("sectionGroupSchemaJson carries each section's title, action and binds", "[sections]") { auto const json = morph::forms::sectionGroupSchemaJson(); REQUIRE_FALSE(json.empty()); @@ -172,7 +198,7 @@ TEST_CASE("sectionGroupSchemaJson carries each section's title, action and binds TEST_CASE("SectionSet: sections fire independently, in any order", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; morph::forms::SectionSet sections{handler}; @@ -183,20 +209,23 @@ TEST_CASE("SectionSet: sections fire independently, in any order", "[sections]") // std::logic_error -- "field belongs to an action that is not the current // step" -- which is exactly the gap morph#513 reports. sections.set<&SecPrefs::theme>("dark"); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + drain(cbExec); // Then the first. Both fire; neither was ever "current". sections.set<&SecProfile::name>("ada"); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); + drain(cbExec); auto const fired = recorder().snapshot(); + REQUIRE(fired.size() == 2); CHECK(fired[0] == "SectionsTest_Prefs"); CHECK(fired[1] == "SectionsTest_Profile"); + CHECK(sections.resolved("SectionsTest_Prefs.summary").has_value()); + CHECK(sections.resolved("SectionsTest_Profile.id").has_value()); } TEST_CASE("SectionSet: a not-ready draft is not sent at all", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; @@ -214,14 +243,16 @@ TEST_CASE("SectionSet: a not-ready draft is not sent at all", "[sections]") { // SecPrefs::validate() requires a non-empty theme; profileId alone is not ready. sections.set<&SecPrefs::profileId>(7); - CHECK(morph::testing::waitUntil([&errors] { return errors.load() != 0; }, std::chrono::milliseconds{300}) == - false); + CHECK(cbExec.pending() == 0); + CHECK(cbExec.runAll() == 0); + CHECK(errors.load() == 0); // Completing it dispatches, carrying the field set earlier, and succeeds. // Waiting on the capture rather than on the recorder: the recorder is // written inside execute(), which runs before the completion that captures. sections.set<&SecPrefs::theme>("light"); - REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Prefs.summary").has_value(); })); + drain(cbExec); + CHECK(sections.resolved("SectionsTest_Prefs.summary").has_value()); CHECK(recorder().snapshot().size() == 1); CHECK(errors.load() == 0); CHECK(sections.resolved("SectionsTest_Prefs.profileId") == std::string{"7"}); @@ -229,24 +260,28 @@ TEST_CASE("SectionSet: a not-ready draft is not sent at all", "[sections]") { TEST_CASE("SectionSet: an already-fired section fires again on the next edit", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; morph::forms::SectionSet sections{handler}; recorder().clear(); sections.set<&SecProfile::name>("ada"); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + drain(cbExec); + REQUIRE(recorder().snapshot().size() == 1); // No latch: the draft is still ready, so it dispatches again. Matching // FlowSession, which also re-fires a ready step on every set<>. sections.set<&SecProfile::name>("grace"); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 2; })); + drain(cbExec); + CHECK(recorder().snapshot().size() == 2); + // The second dispatch really carried the new value, not a replay of the first. + CHECK(sections.resolved("SectionsTest_Profile.name") == std::string{R"("grace")"}); } TEST_CASE("SectionSet: reset clears one section and leaves the others intact", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; morph::forms::SectionSet sections{handler}; @@ -264,18 +299,22 @@ TEST_CASE("SectionSet: reset clears one section and leaves the others intact", " // Per-section isolation is the point: resetting one editor must not wipe // what the user typed into another. CHECK(sections.draft().name == "ada"); + + // The profile dispatch above is still outstanding; let it land before the + // set is destroyed. + drain(cbExec); } TEST_CASE("SectionSet: a fired section's fields are resolvable; an unfired one is not", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; morph::forms::SectionSet sections{handler}; recorder().clear(); sections.set<&SecProfile::name>("ada"); - REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + drain(cbExec); // The result's field: SecProfileResult::id == name.size() == 3. CHECK(sections.resolved("SectionsTest_Profile.id") == std::string{"3"}); @@ -290,7 +329,7 @@ TEST_CASE("SectionSet: a fired section's fields are resolvable; an unfired one i TEST_CASE("SectionSet: a failing dispatch reaches the onError callback", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; @@ -300,32 +339,48 @@ TEST_CASE("SectionSet: a failing dispatch reaches the onError callback", "[secti recorder().clear(); sections.set<&SecExplodes::label>("boom"); - REQUIRE(morph::testing::waitUntil([&] { return errors.load() == 1; })); + drain(cbExec); + CHECK(errors.load() == 1); // A section that succeeds does not route to onError, so the count above is // the failure and not merely "some callback ran". sections.set<&SecProfile::name>("ada"); - REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + drain(cbExec); + CHECK(sections.resolved("SectionsTest_Profile.id").has_value()); CHECK(errors.load() == 1); } TEST_CASE("SectionSet: an unhandled failure logs instead of escaping", "[sections]") { morph::exec::ThreadPoolExecutor pool{2}; - morph::testing::InlineExecutor cbExec; + morph::testing::StepExecutor cbExec; morph::bridge::Bridge bridge{std::make_unique(pool)}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; - // No onError: the failure has nowhere to go but the log. It must not - // propagate out of the completion and take the executor thread down. - morph::forms::SectionSet sections{handler}; - - recorder().clear(); - REQUIRE_NOTHROW(sections.set<&SecExplodes::label>("boom")); - REQUIRE(morph::testing::waitUntil([&] { return recorder().snapshot().size() == 1; })); + // Capture the log rather than only asserting nothing threw: the logged + // line is also the signal that the error continuation has finished, which + // is what lets the set be destroyed safely at the end of this case. + std::atomic logged{false}; + morph::log::ScopedLoggerOverride const logGuard{[&logged](morph::log::LogLevel, std::string_view msg) { + if (msg.contains("SectionsTest_Explodes")) { + logged.store(true); + } + }}; - // The set survives its own failed section: an unrelated one still works. - sections.set<&SecProfile::name>("ada"); - REQUIRE(morph::testing::waitUntil([&] { return sections.resolved("SectionsTest_Profile.id").has_value(); })); + { + // No onError: the failure has nowhere to go but the log. It must not + // propagate out of the completion and take the executor thread down. + morph::forms::SectionSet sections{handler}; + + recorder().clear(); + REQUIRE_NOTHROW(sections.set<&SecExplodes::label>("boom")); + REQUIRE_NOTHROW(drain(cbExec)); + CHECK(logged.load()); + + // The set survives its own failed section: an unrelated one still works. + sections.set<&SecProfile::name>("ada"); + drain(cbExec); + CHECK(sections.resolved("SectionsTest_Profile.id").has_value()); + } } TEST_CASE("SectionSet: destroying it with a dispatch in flight delivers nothing", "[sections]") { From 70ee4f2d9993e6b254035b903e42ebb75d480c6d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 22:33:13 +0200 Subject: [PATCH 11/13] docs: spell out when a SectionSet may be destroyed The Windows segfault came from believing resolved() returning a value meant the completion had finished. It does not -- capture publishes each key as it writes it. Say so where a reader looking for the teardown rule will find it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/forms/sections.md | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/docs/spec/forms/sections.md b/docs/spec/forms/sections.md index 224577db..c77d6557 100644 --- a/docs/spec/forms/sections.md +++ b/docs/spec/forms/sections.md @@ -186,9 +186,20 @@ Every continuation is gated on one `morph::async::CallbackScope` ([../core/callback_scope.md](../core/callback_scope.md)), declared last so it is the first member destroyed, and stopped explicitly at the top of `~SectionSet`. A completion resolving after the set is gone finds the token -stopped and returns without touching anything. As always, how strong that -guarantee is depends on which thread destroys the set — destroying it off the -delivery thread is advisory, and that caller owns its own synchronisation. +stopped and returns without touching anything. + +That covers a completion which has not yet started. It does not cover one +already past its token check: `requestStop()` does not wait, by design. So a +`SectionSet` may only be destroyed while a dispatch is outstanding if the +destroying thread is the one completions are delivered on — the ordinary case +for a UI-thread callback executor, and the boundary +[../core/callback_scope.md](../core/callback_scope.md) describes. + +**Polling `resolved()` is not a substitute for that.** Capture publishes each +key as it writes it, so a value becoming visible means the completion has +started, not that it has finished. A caller that destroys a set on one thread +the moment a value appears on another is destroying it mid-callback. `tests/test_sections.cpp` demonstrates the safe shape: deliver the completions on the +thread that owns the set. Unlike `FlowSession`, nothing here is keyed to a current position, so a reply arriving late cannot be *stale*: there is no position for it to be stale @@ -269,6 +280,10 @@ make a member field wrong to set. - Destroying the set with a dispatch genuinely in flight (a section whose model call blocks until the test releases it) delivers nothing afterwards. +Every other case delivers its completions on the test thread through a +`StepExecutor` and drains before the set leaves scope, for the reason +[Concurrency and lifetime](#concurrency-and-lifetime) gives. + ## Cross-references - [workflows_navigation.md](workflows_navigation.md) — `FlowSession` and the From ad90e03cca0a2e88063623eeabc6aad2ddd9e675 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 22:34:22 +0200 Subject: [PATCH 12/13] forms: don't overclaim why the write_json guard's false arm is untested "Unreachable, since the same writer already served schema generation" is not true: schema generation writes the DOM, not the member, so it is not evidence about this call. Use flows.hpp's honest wording -- the arm is untested because these action fields are plain data the writer cannot fail on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- include/morph/forms/sections.hpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/include/morph/forms/sections.hpp b/include/morph/forms/sections.hpp index acd80335..30a2ab7b 100644 --- a/include/morph/forms/sections.hpp +++ b/include/morph/forms/sections.hpp @@ -260,10 +260,12 @@ class SectionSet { value, [&](std::string_view name, const auto& member) { static_cast(I); std::string json; - // Mirrors flows.hpp: a field glaze cannot write is skipped - // rather than stored as a broken string. The false arm is - // unreachable for any type that got this far, since the - // same writer already served schema generation. + // The `!write_json(...)` guard's false arm (write failure) + // is not exercised by this file's own test suite: every + // section action's fields are plain, well-formed data + // glaze's JSON writer cannot fail on. The same + // "untestable line" flows.hpp's captureResult already + // carries, for the same reason. if (!glz::write_json(member, json)) { _resolvedValues[std::string{typeId} + "." + std::string{name}] = std::move(json); } From 0190b8c27715710a4459d1955da4617508c03468 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Thu, 10 Sep 2026 23:49:56 +0200 Subject: [PATCH 13/13] docs: record where execution diverged from the sections plan The plan's test steps wait on resolved() before leaving scope, which is the exact shape that segfaulted on Windows -- a reader following them verbatim would reintroduce it. Say so at the top, with the two other corrections and the FILE_SET gate the plan missed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- .../plans/2026-09-10-unordered-sections.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/superpowers/plans/2026-09-10-unordered-sections.md b/docs/superpowers/plans/2026-09-10-unordered-sections.md index f489eae4..a4177ef1 100644 --- a/docs/superpowers/plans/2026-09-10-unordered-sections.md +++ b/docs/superpowers/plans/2026-09-10-unordered-sections.md @@ -14,6 +14,38 @@ declaration types (`Section`, `SectionGroup`), the schema emitter, and the runti **Tech Stack:** C++23, header-only. Catch2 v3 for tests. glaze for JSON. No new dependencies. +## Where execution diverged from this plan + +Recorded because the task steps below are wrong in one way that matters, and a +reader following them verbatim would reintroduce a real bug. + +1. **The tests here wait on `resolved()` before leaving scope. That is not + safe.** `captureResult` publishes its first key while still writing the + rest, so a value becoming visible means the completion has *started*, not + finished — and `CallbackScope::requestStop()` does not wait for a callback + already past its token check. A test that polls `resolved()` on one thread + and then destroys the set tears it down mid-callback. It segfaulted on + Windows CI and TSan reproduces it about once in fifteen runs. As shipped, + every case except the lifetime one uses `StepExecutor` and an explicit + `drain()`, so the thread that delivers the completion is the thread that + destroys the set. See `docs/spec/forms/sections.md`, "Concurrency and + lifetime". + +2. **Task 3's "not-ready draft" test measured nothing.** It checked the + recorder immediately after an incomplete `set<>`; dispatch is asynchronous, + so it passed with or without the gate. The shipped test watches `onError` + instead — `BridgeHandler::execute` enforces `ActionValidator` on its own + path, so a missing section-level gate shows up as a spurious validation + failure, not as a bad execution. + +3. **A gate this plan missed:** `CMakeLists.txt` fails configure for a public + header that belongs to no target's `FILE_SET HEADERS`, so + `include/morph/forms/sections.hpp` had to be added there (morph#230). + Headers under `detail/` are exempt. + +4. **`app.hpp` reached into `flows::detail`** for a walker, so Task 1 had to + repoint it at the shared header as well. + ## Global Constraints Every task's requirements implicitly include these. They are the gates CI applies.