ARCADIA-Studio/harness/reviews/scenario-text-union-source-20260923.md

Scenario/Text union: independent source review

2026-09-23 — delegated Astra critic. Local-only report; parent owns compilation, fixes, integration and publication.

Updated disposition: correction 0b61771d079cae4a513958fad04e6c3e785d458f passes the targeted source re-review and resolves the inherited P2 finding in source. No source-review blocker remains in the inspected writer/default, Text admission or always-active-canvas composition. Proceed with the parent's frozen compiler/test gates. No runtime, native GUI, cross-platform or release acceptance is claimed. The original finding below is preserved against 6f4dfc6.

Source and review scope

Reviewed clean .local/scunion at 6f4dfc67b3443b2b1725e2b576eb37224977b9b2 against root 5960563b23b3f28f83343c283b7c156bf248994a, and compared relevant seams with Scenario/inspector 14e7e6aa46f59346c7a9a478fc9498fd0b496aaf (which includes b10). Read harness/scenario-text-union-20260923.md, .local/september23-peer-integration-review.md, actual source deltas and pertinent test bodies. Independent git diff --check 5960563 6f4dfc6 returned no whitespace errors.

This is a focused composition review, not a fresh complete audit of the 14,000-line saved-Scenario contribution. No compiler, tests, GUI, source edits, author-branch mutation or Gitea action occurred. All paths/line numbers below are relative to candidate 6f4dfc6.

Correction re-review

Independently verified clean .local/scunion HEAD at 0b61771d079cae4a513958fad04e6c3e785d458f and inspected its complete four-file delta from 6f4dfc6. Production change is the 11-line early epoch condition/comment in app.rs; the other changes add the regression, recapture the current post-viewport context in an existing test, repair an obsolete focus-mode test comment and record the correction. git diff --check 6f4dfc6 0b61771 returned no whitespace errors. No compiler/test was run by this critic.

The early gate now requires current canvas epoch for all active Scenario commands before shared reconciliation. The enumerated FormShown/Viewport/Restored/Scrolled/Camera exceptions match the inspected existing inner-handler admission contracts; Camera still checks its separate lifetime and spacing still uses its separate early envelope. Existing document/owner/representation/visit/revision checks remain. Production BehaviorControl can still refresh an otherwise admitted toolbar epoch before redispatch. This addresses the actual ordering defect without removing the intentional passive-event behavior.

The new stale_active_actions_cannot_remove_new_same_visit_deferred_reveal is discriminating: it captures the old Context, performs real same-view RevealInDiagram, asserts unchanged visit/representation/revision and changed epoch, then sends old Representation/Shelf/Select/Zoom messages. It compares the complete pending tuple, pending visit, navigation focus, camera boundary, current Context, selection, accepted source/model, source workspace and history after each refusal. The original 6f4dfc6 trace would clear the tuple on the first Representation event. Finally a current viewport event must consume the receipt while retaining target focus/selection and accepted document/history, proving the guard does not merely disable reveal completion. These are independently specified state invariants, not expected values computed by the new predicate. Execution remains pending.

The existing prior-visit spacing test's newly recaptured post-Viewport Context is also correct: a successful viewport update can increment the canvas epoch, so its later preserved-current-state oracle must use the Context after that update. No acceptance assertion was removed by this adjustment.

Verified finding

P2 — stale active Scenario input from the same visit clears a newer deferred reveal before being rejected. Inherited from 14e7e6a, not introduced by this union.

At crates/oa-desktop/src/app.rs:1380–1391, early Scenario admission checks document, owner, representation, visit and revision, but does not require the canvas epoch for ordinary active commands. Only Camera lifetime and Properties receive additional early checks (:1393–1412). reconcile_pending_diagram_reveal runs at :1423; its implementation at app/diagram_reveal.rs:87–113 clears pending reveal, pending Scenario visit and navigation focus for a Representation command. The exact context/epoch refusal happens later at app.rs:2128–2130, after those effects.

Concrete source trace:

1. In an already open saved Scenario, capture CanvasEvent::Representation(old_context, current_representation) from the arrangement picker.
2. Reveal another object in the same representation before processing that captured callback. behavior/scenario.rs:390–417 retains the current visit when document/owner/representation are unchanged. behavior/scenario/usage.rs:248 and app/scenario_diagram.rs:7–38 use that entry behavior. app/diagram_usages.rs:463 increments canvas_epoch, then :479–506 installs a new navigation focus and pending reveal with the retained visit.
3. Deliver the old Representation event. It passes the early gate because only epoch differs, clears the newer reveal/focus in reconciliation, then fails exact context in the inner handler. Its requested action is refused but the pending navigation is already lost.

This is a deterministic control-flow finding; it was not executed by this critic. The same early admission and delayed exact-context ordering are present in inspected 14e7e6a. Root adds allocation preflight alongside these shared boundaries but does not create the underlying reveal defect.

Fix: before shared boundary effects, require exact current Context for active Scenario commands, including Representation/Select/OpenUse/Back/tool/reorder actions. Keep only explicitly documented passive exceptions for Viewport, FormShown, Scrolled and Restored, plus Camera's separate lifetime contract and the already separately admitted spacing protocol. Preserve the production BehaviorControl policy that deliberately refreshes an admitted toolbar epoch; do not remove its same-camera-lifetime behavior indiscriminately.

Discriminating regression: capture the actual Representation message, reveal another object in the same saved view, assert same visit but new epoch and live pending reveal/focus, deliver the captured message, then require every pending receipt/focus/context/selection/camera/document/history value unchanged. Finally send the current viewport event and assert the reveal completes normally. Existing scenario_spacing/tests.rs:294–315 covers stale spacing finish through its separate envelope, and the newly added union test at :29–89 changes visit; neither isolates this active-command same-visit hole.

Writer/default composition accepted within scope

crates/oa-core/src/source_project.rs:853–872,930–968 retains CompleteForEdit as old/new comparison authority and separate Compact/CompactValues insertion templates. It does not replace current root formatting with the older Scenario writer or globally add a null layout default.

EditContext (:1239–1256) grants Scenario row semantics only along Diagram → layout → Scenario → lanes/messages. scenario_item_key (:1655–1696) uses stable alias-to-original-identity correspondence for lifelines, and the full referenced occurrence path plus occurrence for messages; contextual paths do not collapse to the final occurrence alone. Existing identity_origins feed the alias mapping before list matching, so rename/reidentity correspondence survives this composition. Context deliberately returns to Other inside a matched row, avoiding accidental Scenario keys on unrelated nested lists.

The narrow explicit-none exception (:1462–1470) preserves an authored layout none only when the complete original model already had no layout field. A real Some→None clear instead erases significant layout tokens while retaining trivia through the existing erase_syntax behavior. It does not turn absent layout into an empty Some payload. No conceptual defect was found in these inspected rules.

The new scenario_layout/tests/source_lifecycle.rs:248–302 is a meaningful combined oracle: authored explicit-empty description survives name/alias changes; omitted categories/view_options stay omitted; lane/message row comments follow swaps; unchanged numeric spelling remains; clear retains comments; decoded Project equals explicit intended mutations; repeating the clear is byte-exact no-op. Existing tests at :43, :88 and :218 independently cover rename/reorder, reidentity/comment correspondence, explicit-none→Some→None and full model equality. Assertions are targeted authored-byte fragments rather than a full expected-file literal, so this evidence does not establish preservation of every unrelated byte; existing ordinary default-preservation and whole-bundle tests remain required execution gates.

Input authority and always-active canvas accepted within scope

The union keeps TextWorkspace dispatch and source/diagram-usage/allocation envelopes before shared boundaries (app.rs:1334–1360), early scoped toolbar refusal, separately admitted Scenario spacing (:1362–1364), and root rename-envelope/preview-scroll handling (:1367–1376). Scenario visit/representation and selected-Properties guards are composed before the shared effects, subject to the narrower epoch finding above. WorkDrag KeyboardBoundary/Unfocused remain passive exceptions in app/diagram_reveal.rs:102–103.

behavior/canvas_workspace.rs:68–92 carries Scenario representation and visit in Scope; app.rs:1473–1517 compares input ownership, limits toolbar messages to supported presentation controls, refreshes only the admitted context epoch, and preserves spacing retirement and logical focus behavior. No obsolete scenario.focused, scenario.focus, CanvasEvent::Focus, or panel-focus references were found in candidate desktop source. This is source compatibility evidence, not a compiler or keyboard usability result.

behavior/scenario.rs:534–538 makes Details→Properties dismiss the Scenario shelf only; app.rs:2187–2193 selects the Properties tab without selecting/applying another record or changing representation/camera. Inspector composition in views.rs:881–908 preserves the two-child outer arrangement with Scrollable at child 1, retaining the reviewed Properties/Connections reconciliation shape while placing Apply/Revert outside the scroller. The adapted mounted tests appropriately replace obsolete hide/restore-panel assertions with actual editable inspector geometry, same view/visit/selection/camera and unchanged accepted snapshot checks. Removing focus-mode assertions is consistent with root's intended always-active canvas; it is not evidence that real keyboard focus is correct.

The new stale Text/Scenario union test retains an actual spacing token/stamp, source version, complete source/model, selection, camera and history after queued Text indentation/drawer/command messages and earlier-visit Scenario commands, then finishes the still-live gesture. Those are useful independent state oracles. Its final can_undo assertion proves a commit occurred but does not alone verify the exact coordinate delta; adjacent existing same_batch_preview_is_immutable_and_one_commit_undo_redo_has_exact_source_parity supplies an independently calculated expected Project and exact source Undo/Redo checks. Run both.

Acceptance remaining

Execute the now-authored same-visit stale-command regression at the corrected pin, then run the parent's actual frozen compiler/test inventory, ordinary default/source lifecycle, Scenario source/references/layout, Text/source indentation/rename-scroll, spacing/Details/navigation and focus suites. Inspect union-only constructor/API failures before inferring anything from historical totals.

Native combined acceptance remains required for arrangement picker → Details → lower-field Apply/Revert → Properties/Connections return → spacing/cancel → Text Tab/edit/Apply/return → save/reopen, preserving view/visit and logical keyboard ownership. Source inspection and mounted test code cannot establish that workflow's usability. Attachment, transition, resize, private source8 and release/platform acceptance remain separate work.