Framework comment audit: the remainder — ~30 further stale comments, and one paragraph copy-pasted 18 times

[Yaraslaut]

What this is

The residue of a twelve-way audit over all 64 headers in include/morph (28,496 lines). PR #503 fixed the ~30 stale comments verified by reading the code they describe, plus 10 mirrored spec passages. This records what it did not fix, so the work is not lost — per AGENTS.md, "a defect noticed in passing and left unrecorded is a defect nobody will find again on purpose".

Code defects found by the same audit are filed separately: #493–#502.

Verification status

Mixed, stated per item below. These are agent findings I have not independently re-verified, unlike the ones in #503. Each cites a file and line; treat them as leads with evidence attached, not as established facts. Anyone picking one up should confirm it first — the audit's own hit rate was good but not perfect, and one finding in it was wrong about my own prior change until checked.

A. Cross-cutting duplication — the clearest "compression" targets

1. The "public macro surface: hand-aligned on purpose" paragraph appears 17-18 times. Six identical lines, including the anecdote "in one case it broke a token-paste invocation apart". Sites: core/registry.hpp (×8), core/model_key.hpp (×4), util/quantity.hpp (×2), forms/views.hpp, forms/app.hpp, forms/flows.hpp. One canonical statement (CONTRIBUTING.md, or beside .clang-format) plus // clang-format off -- see <ref> at each site carries the same information. Note two of the copies are inapplicable where they sit: quantity.hpp contains no token paste (grep '##' finds none), and one copy guards an empty macro body.
2. The EscapingWriteOpts glaze-7.4 rationale, ×4 plus 2 restatements. core/wire.hpp:340, journal/action_log.hpp:181-193, session/session_auth.hpp:352-365, offline/file_offline_queue.hpp:63-80, then restated at action_log.hpp:211-214 and session_auth.hpp:405-408. The struct duplication is deliberate; the prose is not, and three copies already claim to be mirrors of wire.hpp's.
3. "Unreachable through any real code path", ×4. forms/i18n.hpp:53-58, forms/layout.hpp:54-59, offline/reconnect_coordinator.hpp:36, plus the canonical forms.hpp:618-623 two of them already point at. The pointer alone would do.
4. "Test-only observability" caveat ×3, with a circular cross-reference. core/detail/execute_order_gate.hpp:136-138 → PendingCallTable::size(), core/detail/subscription_registry.hpp:164-166 → ExecuteOrderGate::gateCount(), core/detail/reply_router.hpp:172-175. The two "mirroring" pointers form a cycle that never reaches a canonical statement.
5. core/strand.hpp:85-95 and :165-172 narrate the same historical orphaned-strand bug in full, twice.
6. journal/journal.md's _checkpointMtx rationale ×3 within a 200-line span (journal.hpp:325-327, :423-431, :439-449), plus a fourth in the spec.
7. forms/forms.hpp: thirteen near-identical emitNode() bodies and three membership test() bodies differing only in a final comparison — code duplication rather than comment duplication, but the same drift risk.

B. Stale comments not yet fixed

Each VERIFIED by the auditing agent against the code, not re-verified by me.

• core/remote.hpp:1604-1635 — the _executeGate rationale is stated twice, and the second copy contradicts itself mid-sentence ("never touches this gate at all... except it does get a ticket"). The code settles it: handleImpl:355-358 takes a ticket for any well-formed execute.
• core/completion.hpp:56-58 — the "no handler sees a moved-from value" guarantee holds within one dispatch, not across a later then() re-attach; docs/spec/core/completion.md:288-291 documents the real behaviour, and the spec is internally inconsistent with itself at :106-108.
• core/logger.hpp:275-281 — calls an unlocked read of minLevel a "data race"; it is std::atomic. The real reason to lock is snapshotting sink+level as a pair.
• core/payload_schema.hpp:136-149 — the grammar table's row order contradicts the if constexpr dispatch order, forcing the paragraph below it to correct the table.
• core/detail/reply_router.hpp:93-95 — "so the two cannot be allocated and stored under different locks" describes a relationship that does not exist; _nextCallId is a lock-free atomic outside _mtx.
• core/detail/execute_order_gate.hpp:90 — "should not happen" contradicts the file's own contract 63 lines above and a test named for that exact behaviour.
• core/callback_scope.hpp:99-102 — "rejected at compile time" only holds if the returned wrapper is invoked; the static_assert is inside the lambda body.
• core/callback_scope.hpp:182-187 — drops the spec's load-bearing morph#486 caveat that the header is the natural place to state.
• core/backend.hpp:925-929 and :736-738 — _changeAware insertion lives in createAndTrack, which registerModelShared calls directly; neither comment names it. Mirrored at docs/spec/core/backend.md:1791.
• core/backend.hpp:604-609 — @p used for two things that are not parameters of that function.
• core/registry.hpp:468-470 — claims every entry is fingerprint-stamped; actionPayloadSchema's own doc 240 lines up documents the unstamped case, which a test exercises.
• core/registry.hpp:454-462 — omits enforceQuantityBounds from the runner's pre-execute steps, which is the untrusted-envelope check the paragraph exists to enumerate.
• core/model.hpp:12 — #include "strand.hpp" is unused; removing it exposes a latent missing <concepts>.
• core/wire.hpp:44-68 — the Envelope discriminator list omits "hello", which makeHello() produces; docs/spec/core/wire.md:37 lists it.
• core/wire.hpp:568-585 — doc says LegacyPeer requires an "err" kind; the code tests the message string alone.
• forms/forms.hpp:668, 722, 1206, 1657 — four "added in a later task" markers for code defined above the reader.
• forms/instance_constraints.hpp:24-27 — the file header says decorate() rewrites the framework's own bound keys; it adds x-minimum/x-maximum alongside the compiled minimum/maximum, as the function's own @brief and two spec sections say.
• forms/instance_constraints.hpp:134-137 — "callers clamp to the same range before calling this"; the one caller rejects rather than clamps.
• forms/views.hpp:314-315 — "schema generation never throws" drops the UnsatisfiableFormError the spec qualifies it with.
• forms/layout.hpp:41-44 — cites a "renderer's documented downgrade" no document contains.
• forms/app.hpp:79-84 — names a deleted docs/planned/ file and a ViewTraits<V> that already exists, so it reads as blocked work when nothing blocks it.
• util/quantity.hpp:735 — names an operator<< the same file says it does not provide.
• util/quantity.hpp:738-739 — "above" points ~85 lines below.
• util/rational.hpp:898-901 — the zero-guard's stated reason is wrong (gcd(0,d) == d, never 0 here); the sibling mulAssignUnchecked omits the guard entirely.
• util/rational.hpp:98, 111 — <atomic> and <stdexcept> unused; <string_view> used but not included.
• qt/qt_websocket_backend.hpp:92-94, 205-209, 326-328 — "callbacks fire on the Qt event-loop thread once the reply arrives" is narrower than the code: three paths invoke them inline on the caller's thread, and cancelPending fires them from whatever thread destroys the bridge. This one matters — core/backend.hpp's threading contract (added in bridge: record what actually closes the *Async reply sites' UAF window (#489) #492) leans on it.
• qt/qt_websocket_backend.hpp:517-521 — "needs no cancellation bookkeeping" is contradicted by :218-222 and by cancelPending's actual queue drain.
• net/detail/ws_handshake.hpp:270 — the "64 KiB safety cap" is checked before a 4 KiB read, so the real bound is ~68 KiB; the in-tree test says so and the comment does not.
• net/socket_server.hpp:43-45 — says replies are "marshalled back onto the owning connection's own write path"; they are written inline on the pool thread under writeMtx, as the spec correctly states.
• session/session.hpp:96 + docs/spec/session/session.md:416 — covered by session/security: IAuthorizer::authorize also gates 'instances' and 'schemas' with an empty actionType, but is documented as execute-only #500.
• journal/session_auth.hpp:385, 435, 533 — cite a docs/spec/security.md section title that does not exist (three sites, plus docs/todo.md:186).
• offline/offline_queue.hpp:129-135 — states the dedup contract as universal ("the call succeeds"); SqliteOfflineQueue can throw OfflineQueueFullError on a dedup hit, as its own @throws documents.
• offline/sync_worker.hpp:55-59, 203 — SyncResult's accounting claim is false on the stop() path, and @return omits the fourth field.

C. Consistency, not correctness

• [[nodiscard]] missing where the return value is the contract: Bridge::registerHandler/registerSharedHandler (discarding destroys the binding and leaks the backend-side model), the four IBackend::*Async hooks, detail::parkIfInFrame/claimHandoff, and every pure factory in wire.hpp (13 of them, while three neighbours have it).
• bool parameters in public API surface, against the repo's own guideline: TraceSink::endSpan(SpanId, bool ok), views::describeAction(..., bool confirm), buildActionNode(..., bool includeButtonFields), IModelHolder::setOutboxManaged(bool).
• Heterogeneous lookup missing on hot paths that allocate a std::string per lookup: registry.hpp dispatch (2 allocations per remote action), flows.hpp:421, views.hpp:426.
• docs/CMakeLists.txt EXCLUDE_SYMBOLS omits morph::flows::detail and morph::wire::detail while excluding every other detail namespace — so those two publish internals as API docs. Looks unintentional; either the list or its "hide every per-topic detail namespace" comment is wrong.

What would change the verdict

Each item is independently checkable against the cited line. Close individual items by fixing or refuting them; close this issue when the list is empty or the remainder is judged not worth carrying. It is deliberately one issue rather than thirty — none of these is individually worth a tracker entry, and the value is in having the sweep recorded somewhere it can be worked down.

[Yaraslaut]

Triage: triage: unverified, area: docs

unverified is the honest label, and it is the issue's own claim about itself: "These are agent findings I have not independently re-verified … treat them as leads with evidence attached, not as established facts." Each item is blocked on a cheap experiment — read the cited line and confirm — not on a decision. That is exactly what this label is for, and it stops a future reader treating the list as a settled backlog.

Not parked: nothing here is deferred pending a trigger; the work is available now, item by item.

The list is already provably imperfect, which is the point of the label. Triage found that six items it lists as outstanding — the four forms.hpp "added in a later task" markers and two docs/planned/ paths — were claimed as fixed by PR #503's own description but were not. They are fixed now (7f535f82), and grep -rn "docs/planned\|later task" include/ is empty. If the list was wrong about those in the direction of under-reporting progress, it can be wrong in the other direction too. Confirm before acting on any entry.

Two items have since moved out of this list entirely and should not be worked from here:

• session/session.hpp:96 → tracked as session/security: IAuthorizer::authorize also gates 'instances' and 'schemas' with an empty actionType, but is documented as execute-only #500 (rescoped: the code is right, the IAuthorizer contract is what needs correcting).
• net/socket_server.hpp:43-45 and the ws_handshake.hpp:270 cap are still in scope here, but the SocketServer reaping defect is net: SocketServer never reaps finished connections, leaking an fd and a thread handle per connection ever accepted #498, not a comment fix.

Section C is a different kind of item and may deserve splitting. The missing [[nodiscard]]s, the bool parameters in public API surface, and the missing heterogeneous lookup are code changes with behavioural and ABI consequences, not comment corrections. Two of them are load-bearing rather than cosmetic — discarding Bridge::registerHandler's return destroys the binding and leaks the backend-side model, and discarding an IBackend::*Async return means no registration ever happens. Those two arguably belong in their own bug-labelled issue; the rest of C is genuine polish. Leaving the split to whoever picks it up, rather than fragmenting further now.

Invariant 6 applies to section A and is the strongest argument in the whole issue. The "public macro surface" paragraph appearing 17-18 times is well past the rule-of-three trigger, and unlike most duplication it is provably drifting: two of the copies are inapplicable where they sit (quantity.hpp contains no token paste — grep '##' finds none — and one copy guards an empty macro body). That is the item most worth doing first, and the one least likely to be wrong.

[Yaraslaut]

Partially addressed in PR #509 — including one item that was wrong

Section A1, the biggest compression target, is done. The "public macro surface" paragraph was copy-pasted 17 times across six headers. Collapsed to one canonical statement in CONTRIBUTING.md under Formatting/linting, with a one-line pointer at each site.

Worth recording why it was worth doing beyond tidiness: the paragraph claims the formatter "broke a token-paste invocation apart", and four of the six files contain no ## at all — the copies had drifted into describing code that was not there, which is this issue's own subject.

Eight further stale comments fixed, all verified against the code first: wire.hpp's discriminator list omitting "hello" (which makeHello() produces) and its LegacyPeer "err"-kind claim; execute_order_gate.hpp's "Defensive; should not happen" over a branch its own file-level contract lists as first-class and a named test exercises; logger.hpp calling an unlocked read of an atomic a data race; backend.hpp's _changeAware maintenance attribution; reply_router.hpp's "different locks"; ws_handshake.hpp's 64 KiB cap; socket_server.hpp's "marshalled" threading claim.

One item was reported as a defect and is not one

Section B lists model.hpp:12's #include "strand.hpp" as unused, noting that removing it would expose a latent missing <concepts>. The second half is right. The first is right only about model.hpp itself — removing it broke tests/test_model.cpp, which reaches morph::exec::detail::ModelId through it.

For a header-only public library, dropping a transitive include is a source-breaking change for consumers, for negligible benefit. The include stays, now with a comment saying so; the <concepts> gap is fixed independently.

This is the second time an item on this list has turned out to point the wrong way (the first being the six "later task" markers, which PR #503 had claimed as fixed and had not). The triage: unverified label is doing real work — confirm each entry before acting on it.

The rest of the list stands.