Skip to content

refactor(sessions): extract CurrentTurnScope + drop dead _approvalTurnState (superseded by #1508) - #1502

Closed
Aaronontheweb wants to merge 4 commits into
netclaw-dev:devfrom
Aaronontheweb:refactor/session-current-turn-scope
Closed

refactor(sessions): extract CurrentTurnScope + drop dead _approvalTurnState (superseded by #1508)#1502
Aaronontheweb wants to merge 4 commits into
netclaw-dev:devfrom
Aaronontheweb:refactor/session-current-turn-scope

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Collaborator

Continues the LlmSessionActor decomposition started in #1496 (transient-state handlers, steps 1–4). This is step 5 plus an enabling cleanup. Two commits of substance, no wire/behavior change — every touched field is transient (rebuilt on recovery), and no persisted event/snapshot shape changes.

1. Remove the dead _approvalTurnState breadcrumb

_approvalTurnState and its five-subtype ApprovalTurnState hierarchy were write-only: a full-repo grep confirmed no subtype payload was ever read to make a decision, and no test referenced it. Every reference was a self-referential read (used only to build the next breadcrumb value) or a pure write; its only live side effect was a _currentTurnContext write piggybacked inside the mutators.

Removed the field and the hierarchy, preserving every _currentTurnContext write (load-bearing, especially the recovery-time restore in ApplyToolApprovalRequested). ClearApprovalTurnState became ClearCurrentTurnContext. Removing the breadcrumb also exposed two genuinely dead calls: MarkApprovalRedrive only re-set _currentTurnContext one line after it was already set, and MarkApprovalRunningAfterRedrive was a no-op on the normal tool-completion path.

2. Extract CurrentTurnScope (step 5)

Grouped the seven ambient "what turn is active and where did it come from" fields — _currentTurnSource, _currentTurnContext, _currentTrustContext, _activeRecall, _activeTurnId, _activeMessageId, _activeChannelType (~40 read sites) — into one transient handler, Sessions/Handlers/CurrentTurnScope, mirroring the step 1–4 container pattern (DiscoveredToolCache, TurnStateTracker).

  • The correlation identity (TurnId/MessageId/ChannelType) is mutated only via Bind(MessageSource?) / Bind(TurnContext), which absorb the former BindTurnTelemetry derivation (including the source-turn-id → message-id → generated-id fallback) and keep the three in lockstep.
  • The actor keeps the CrashContextSnapshot breadcrumb (it uses the actor-owned session id + clock) and the read-only audience/boundary precedence projections + trust derivation, reading _turn.* at the call site.
  • Reset semantics preserved exactly: only TurnContext is nulled at a turn boundary (the others are overwritten each turn) — no blanket Clear() that would change behavior.

Net: the actor loses 8 instance fields (the 7 above + _approvalTurnState). Adds CurrentTurnScopeTests characterizing the turn-id derivation fallback that BindTurnTelemetry previously had no direct coverage for.

A follow-up commit reuses _turn.TurnId directly in ContinueIncomingUserMessage instead of re-generating it via a now-dead ?? new TurnId(IdGen.ShortId()) fallback (surfaced by review — the extraction is what makes the duplicate path obvious).

Verification

  • Full Netclaw.Actors.Tests Sessions suite green (incl. ApprovalRehydrationTests ×19, the primary net for the preserved _currentTurnContext writes, and CompactionIntegrationTests).
  • dotnet slopwatch analyze: no new violations. Copyright headers present.
  • Not triggered (confirmed): OpenSpec, eval suite, system-skills sync, config-schema, smoke tapes — pure transient-state refactor.

… prep)

The _approvalTurnState field and its five-subtype ApprovalTurnState
hierarchy were write-only: no subtype payload was ever read to make a
behavioral decision, and no test referenced them. Every reference was
either a self-referential read (used only to build the next breadcrumb
value) or a pure write. Their only live side effect was a
_currentTurnContext write piggybacked inside the mutators.

Remove the field and the hierarchy, preserving every _currentTurnContext
write (load-bearing, especially the recovery-time restore in
ApplyToolApprovalRequested). ClearApprovalTurnState becomes
ClearCurrentTurnContext. Removing the breadcrumb also exposed two dead
calls: MarkApprovalRedrive only re-set _currentTurnContext one line after
it was already set, and MarkApprovalRunningAfterRedrive was a no-op on the
normal tool-completion path.

No persisted state changes (the field was transient and never
serialized); event/snapshot shapes are untouched.

Prep for extracting CurrentTurnScope (step 5).
…p 5)

Group the seven turn-ephemeral fields the actor carried for "what turn is
active and where did it come from" — _currentTurnSource, _currentTurnContext,
_currentTrustContext, _activeRecall, _activeTurnId, _activeMessageId,
_activeChannelType — into a single transient handler, Sessions/Handlers/
CurrentTurnScope, mirroring the step 1-4 container pattern (DiscoveredToolCache,
TurnStateTracker).

The correlation identity (TurnId/MessageId/ChannelType) is mutated only via
Bind(MessageSource?) / Bind(TurnContext), which absorb the former
BindTurnTelemetry derivation (incl. the source-turn-id -> message-id ->
generated-id fallback) and keep the three in lockstep. The actor keeps the
CrashContextSnapshot breadcrumb (PublishCrashContext) since it uses the
actor-owned session id and clock, and keeps the read-only audience/boundary
precedence projections and trust derivation, reading _turn.* at the call site.

Reset semantics are preserved exactly: only TurnContext is nulled at a turn
boundary (ClearCurrentTurnContext); the other fields are overwritten each turn
as before — no blanket Clear() that would change behavior.

No persisted state changes (all seven fields were transient). Adds
CurrentTurnScopeTests characterizing the turn-id derivation fallback that the
former BindTurnTelemetry had no direct coverage for.

LlmSessionActor field count drops by 8 (the 7 fields + the removed
_approvalTurnState from the prior commit).
…nerating

ContinueIncomingUserMessage built the TurnContext with
'_turn.TurnId ?? new Protocol.TurnId(IdGen.ShortId())', but BindTurnTelemetry
(called one line above) already guarantees _turn.TurnId is non-null — Bind
generates one from the message id or IdGen when the source carries none. The
fallback was therefore a dead branch that forked a second turn-id generation
path, the exact duplication the CurrentTurnScope extraction set out to remove.
Reuse _turn.TurnId directly.
@Aaronontheweb Aaronontheweb changed the title refactor(sessions): extract CurrentTurnScope + drop dead _approvalTurnState (step 5) refactor(sessions): extract CurrentTurnScope + drop dead _approvalTurnState (superseded by #1508) Jun 26, 2026
@Aaronontheweb

Copy link
Copy Markdown
Collaborator Author

Superseded by #1508.

Dropping the CurrentTurnScope extraction: it added a layer of indirection (a new type + a _turn. hop at ~40 call sites) without improving type safety, coherence, or simplification — an anemic container with no behavior or invariant, unlike the step 1–4 handlers it was modeled on. The _approvalTurnState dead-code removal that this PR also contained is unambiguously good and is carried forward on its own in #1508.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant