core+journal: a journal sink that refuses a success no longer makes the framework report — and record — a committed write as rejected (fixes #796)

[Yaraslaut]

Fixes #796.

What was wrong

Both sites that actually run Model::execute called recordActionSuccess from
inside the try whose catch (const std::exception&) exists to report
execution failures — ActionDispatcher::registerAction's runner
(registry.hpp) and Bridge::executeVia's localOp (bridge.hpp).
ActionTraits<Action>::resultToJson sat inside that same try.

IActionLog::append is required to throw when the entry did not reach its
backend: append/flush return void, so it is the only channel the interface
gives an implementation (action_log.hpp:185), and file_action_log.hpp throws
from 18 sites. So a journal file on a full disk produced three wrong answers at
once, after the model's mutation had already committed:

dispatch threw: journal sink unavailable
model.committed = 1, model.balance = 10
journal entries = 1
  outcome=Failed error=journal sink unavailable result=

1. the caller is told a durable write was rejected, and may retry a write that
   landed;
2. the audit trail gains an Outcome::Failed entry for a mutation that
   committed;
3. that entry's error carries the sink's message, permanently blaming the
   action for an infrastructure fault.

What the caller is now told, and why that shape

Model::execute is now the only call inside that try — it is the only one
whose failure means the action was rejected. Serialising the result and
appending the entry run after it, outside.

A throw from either surfaces as a new morph::model::ActionRecordingError:

• what() — "action executed but was not recorded: <cause>"
• cause() — the underlying message, unprefixed
• result() — the committed action's result JSON ("" when serialising it is
  what failed)

Why this and not the obvious alternatives. Catching and discarding was
ruled out by the issue and by OutboxRelay::relay(), which calls
sink->append()/sink->flush() directly and marks a row relayed only after
they return normally — a fix that swallowed would break at-least-once relay.
(OutboxRelay is untouched here; it does not go through either dispatch path.)
Returning normally and logging is the other silent failure: the caller would
believe the action is in the audit trail when it is not, which is exactly the
asymmetry the journal exists to prevent. Letting the sink's exception propagate
raw would keep symptom 1 — the caller cannot tell "the model refused this" from
"the model accepted it and the disk is full", and those call for opposite
actions (retry vs. do not retry).

So the caller still learns the recording failed; what changed is what it is
told, and it is now true: the write happened, the record of it did not.
Deriving from std::runtime_error means no existing handler changes behaviour
— RemoteServer::dispatchExecute's strand catch still turns it into an err
reply, LocalBackend still rejects the Completion through onError — while
a caller that cares can catch the type and read result().

resultToJson moved out too

Yes — it is in the same position and produces the identical three symptoms with
no journal involved (a ParseError from a glaze write error, raised after the
mutation committed).

When resultToJson is what threw, no journal entry is written at all. A
Succeeded entry carries the result by definition (LogEntry::result in
docs/spec/journal/journal.md), and there is none to carry. A committed
mutation with no entry is a real gap, and it is stated in the spec — but it is a
smaller one than an entry asserting the action failed, which is the thing
nothing downstream can distinguish from a genuine rejection.

Both paths

Dispatcher and bridge, each fixed and each tested. The issue recorded the
bridge path as read-but-not-executed; it is now driven by two tests.

Proof, both ways

tests/test_action_log.cpp gains a SuccessRefusingLog — throws on the
Succeeded append, accepts the Failed one, and records every entry it was
offered as well as every one it stored — plus a hand-written ActionTraits
whose resultToJson throws. Five cases.

Each assertion was reddened by mutating the fix back. Real output:

Mutation A — resultToJson + recordActionSuccess returned to the execution
try (master's shape, with ActionRecordingError left declared so the tests
still build). REQUIRE→CHECK for this run so every assertion reports:

ActionDispatcher: a sink that refuses the success append ...
tests/test_action_log.cpp:636: FAILED: CHECK( seen.recordingError )   with expansion: false
tests/test_action_log.cpp:637: FAILED: CHECK( seen.what == "action executed but was not recorded: journal sink unavailable" )
  with expansion: "journal sink unavailable" == "action executed but was not recorded: journal sink unavailable"
tests/test_action_log.cpp:638: FAILED: CHECK( seen.cause == "journal sink unavailable" )  with expansion: "" == "journal sink unavailable"
tests/test_action_log.cpp:639: FAILED: CHECK( seen.result == "10" )   with expansion: "" == "10"
tests/test_action_log.cpp:643: FAILED: CHECK( log->entries().empty() )        with expansion: false
tests/test_action_log.cpp:644: FAILED: CHECK( log->offered().size() == 1 )    with expansion: 2 == 1
tests/test_action_log.cpp:647: FAILED: CHECK( entry.outcome != morph::journal::Outcome::Failed )  with expansion: 1 != 1

Bridge/LocalBackend: a sink that refuses the success append ...
tests/test_action_log.cpp:731-746: the same seven, identically.

ActionDispatcher: a result that will not serialise ...
tests/test_action_log.cpp:696: FAILED: CHECK( seen.recordingError )   with expansion: false
tests/test_action_log.cpp:697: FAILED: CHECK( seen.cause == "result will not serialise" )  with expansion: "" == "result will not serialise"
tests/test_action_log.cpp:702: FAILED: CHECK( log->entries().empty() )        with expansion: false

The balance == 10 / balance == 7 assertions passed under the mutation,
which is the point: the mutation is durable either way, and only what the caller
and the log are told changes.

Mutation B — the regression this fix could cause. The execution catch
changed to throw ActionRecordingError{...} instead of rethrowing:

ActionDispatcher: a genuine Model::execute throw still records Outcome::Failed ...
tests/test_action_log.cpp:670: FAILED: CHECK_FALSE( seen.recordingError )  with expansion: !true
tests/test_action_log.cpp:671: FAILED: CHECK( seen.what == "insufficient funds" )
  with expansion: "action executed but was not recorded: insufficient funds" == "insufficient funds"

Bridge/LocalBackend: a genuine Model::execute throw still records Outcome::Failed ...
tests/test_action_log.cpp:776-777: the same two.

So the three required assertions all fail against the old code, and the
regression guard fails against a plausible over-broad fix.

Gates run locally

• clang-format --dry-run -Werror (clang-format 22.1.8) on all three changed
  C++ files — clean.
• clang-tidy-diff.py (clang-tidy 22.1.8) against origin/master...HEAD, three
  files analysed (the two changed headers pinned into the compile database with
  a real command, as CI's filter does, rather than left to
  InterpolatingCompilationDatabase) — clean, exit 0. Verified non-vacuous:
  a deliberate auto* tidyBait = (const void*)&result; on a changed line in
  registry.hpp was reported as three errors
  (readability-qualified-auto, modernize-avoid-c-style-cast,
  clang-diagnostic-old-style-cast), and removing it returned the run to clean.
• Doxygen --target doc with MORPH_BUILD_DOCUMENTATION=ON and
  WARN_AS_ERROR=FAIL_ON_WARNINGS — exit 0;
  morph::model::ActionRecordingError generates.
• Full ctest on clang-debug (clang 22.1.8, Linux, tests + net,
  examples off): 1782/1782 passed, 76 s.

Not run locally: scripts/coverage.sh — master's coverage leg is red for an
unrelated reason (the comment cleanup moved lines that
scripts/branch_partial_allowlist.json pins; the repair is in #797). That file
is not touched here.

Spec

AGENTS.md makes the spec authoritative, so it moves in the same commit:

• docs/spec/journal/journal.md — new section "A refused recording is not an
  execution failure", plus an invariant bullet stating that a Failed entry
  means the model rejected the action and never that the framework could not
  record a success.
• docs/spec/core/registry.md — registerAction's description no longer places
  the success record inside the execution try; ActionRecordingError added to
  the type list.
• docs/spec/core/bridge.md — the same correction for localOp, and the
  executeVia table row.

Found and left

Nothing filed — nothing found that clears AGENTS.md's bar. One adjacent
observation, recorded here rather than as an issue because it is a property of
this change and not a separate defect: the failure path's own append can still
throw (the issue lists this as unverified). It propagates from inside the
execution catch, replacing the model's exception with the sink's — the same
class of confusion, on the path where the action genuinely was rejected, so no
committed write is misreported. Narrowing it would mean deciding which of two
real failures the caller hears about, which is a different design question from
this one and is deliberately left alone here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!