Skip to content

refactor(channels): extract access-policy gate from _handle_message_inner (#1026)#1036

Merged
vybe merged 2 commits into
devfrom
refactor/1026-message-router
Jun 3, 2026
Merged

refactor(channels): extract access-policy gate from _handle_message_inner (#1026)#1036
vybe merged 2 commits into
devfrom
refactor/1026-message-router

Conversation

@dolho
Copy link
Copy Markdown
Contributor

@dolho dolho commented Jun 3, 2026

Summary

Second refactor from #1026 (after #1035 cleanup-sweeps). Targets the message_router._handle_message_inner hotspot (391 lines, CC 65 per the refactor-audit).

_handle_message_inner is a linear pipeline of ~14 steps. The cross-channel access gate (Issue #311, step 5b) was ~95 lines of group/DM branching inline — the dominant CC contributor and, notably, security-critical code with no isolated unit coverage. This extracts it into _enforce_access_policy(...) -> (allowed, verified_email); the handler now calls the gate and short-circuits on not allowed.

No behavior change. Every branch preserved verbatim:

  • group any_verified: already-unlocked → proceed; sender verifies → set_group_verified + proceed; nobody verified → prompt_group_auth + abort
  • group none: legacy permissive
  • DM require_email + no email → prompt_auth + abort
  • DM verified + has access → proceed; verified + restrictive → upsert_access_request + pending reply + abort; open_access / legacy permissive → proceed

Deny paths still send their prompt/pending response before returning. verified_email is returned for the downstream MEM-001 + session-source steps.

Result

  • _handle_message_inner: 398 → 303 lines, the CC-65 branching block lifted out and isolated.
  • _enforce_access_policy: single-responsibility, ~CC 12.

Tests

Adds tests/unit/test_message_router_access_gate.py8 characterization tests that drive the real _handle_message_inner (fully-mocked adapter + patched collaborators) through every access path, asserting who reaches execute_task vs who is prompted/denied/queued. Green before and after the extraction — proof of behavior preservation, and net-new coverage on the #311 gate.

  • ✅ 8/8 new characterization tests green pre+post
  • ✅ 144 passed across all message_router-adjacent unit tests (vision, file uploads, slack timeout, outbound files) — no regressions

Scope

Per the issue's "small incremental PRs — one function per PR" guidance, this is the first decomposition step for _handle_message_inner. Remaining phases (resolve/token, rate-limit, availability, execute, finalize) are follow-up increments under #1026; the function is not yet under the <100-line / <20-CC target on its own — tracked for the next PRs.

Related to #1026

dolho and others added 2 commits June 3, 2026 12:07
…nner (#1026)

`_handle_message_inner` is a 398-line linear pipeline; the cross-channel
access gate (#311, step 5b) was ~95 lines of group/DM branching inline and
the dominant cyclomatic-complexity contributor (the function scored CC 65).
Extract it into `_enforce_access_policy(...) -> (allowed, verified_email)`.
The handler now calls the gate and short-circuits on `not allowed`.

No behavior change — every branch (group any_verified unlock/lock, DM
require_email prompt, DM verified-no-access pending request, open_access,
legacy permissive) is preserved verbatim; deny paths still send their
prompt/pending response before returning.

Adds tests/unit/test_message_router_access_gate.py: 8 characterization tests
driving the real `_handle_message_inner` through each access path (who
reaches execute_task vs who is prompted/denied/queued). Green before and
after the extraction. The security-critical gate had no isolated unit
coverage before this.

First decomposition step for `_handle_message_inner`; remaining phases
(resolve/rate-limit/execute/finalize) are follow-up increments under #1026.

Related to #1026

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Builds on the access-gate extraction: pull the remaining pipeline phases out
of the 398-line handler into focused methods, leaving a thin linear
orchestrator.

Extracted:
- _resolve_agent_and_token (steps 1–2)
- _maybe_transcribe_voice (2b)
- _enforce_rate_limits (3 + 3b)
- _ensure_agent_available (4)
- _run_agent_task (9 — execution params + failure/exception handling; returns
  result or None)
- _finalize_response (10–14 — persist, MEM-001, outbound files, NO_REPLY,
  send, hooks, cleanup)

Result: _handle_message_inner 398 → 112 lines, cyclomatic ~65 → ~10. Every
early-return gate and the execute/finalize split preserve behavior exactly.

Tests: extends test_message_router_access_gate.py to 16 characterization
cases driving the real pipeline — entry-gate aborts (no agent, no token,
rate-limited DM + group-silent-drop, container down, unverified), execution
failure → error reply without finalize, and happy-path persist+send+hook.
Green before and after each extraction; 152 message_router-adjacent unit
tests pass.

Note: handler is 112 lines (just over the 100-line target); the cyclomatic
target (<20) is met. The remaining ~12-line step-7b file-upload glue is an
optional final trim.

Related to #1026

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dolho
Copy link
Copy Markdown
Contributor Author

dolho commented Jun 3, 2026

/review — positive

Structural pre-landing review (branch diff vs dev, full files read). Behavior-preserving decomposition; 0 critical, 1 informational (pre-existing).

Scope: CLEAN — exactly the #1026 AC item (_handle_message_inner decomposition), two commits (access gate + remaining phases). No unrelated changes.

Control-flow equivalence (the real risk for this refactor) — all preserved

  • Abort signals: _resolve_agent_and_tokenNone, _enforce_rate_limitsFalse, _ensure_agent_availableNone, _enforce_access_policy(False,_), _run_agent_taskNone each short-circuit the handler identically to the original early-returns.
  • State threading: message reassigned from _maybe_transcribe_voice (transcription persists); verified_email flows gate→file-uploads→_run_agent_task_finalize_response; response_text = result.response or "" matches the original.
  • Ordering: is_group computed after voice (same point); voice before rate-limits — preserved.
  • indicate_done placement: inside _run_agent_task on failure/exception, in _finalize_response (step 10) on success — exactly the original split, not double-called.
  • Deny side-effects (prompt_auth / prompt_group_auth / pending send_response) stay inside the gate; group-silent-drop preserved.

Informational

[I1] The step-7b all_writes_failed file-upload abort path has no characterization test (harness sends no files). Not a regression — that block stayed inline, unchanged — but worth a test if 7b is extracted later.

Clean categories

SQL/data safety (db calls verbatim) · concurrency (no new shared state/locks) · auth boundary (#311 gate logic byte-identical, proven by 8 access tests) · credential exposure (bot_token handling unchanged) · dead code (no orphaned handler locals) · error handling (broad except is verbatim) · enum completeness · perf.

Verification

  • 16 characterization tests through the real pipeline (entry-gate aborts, access paths, exec-failure→error-reply-without-finalize, happy-path persist+send+hook) — green before and after each extraction.
  • 152 message_router-adjacent unit tests pass — zero regressions.

Complexity

_handle_message_inner 398 → 112 lines, cyclomatic ~65 → ~10. (112 is marginally over the 100-line AC target; the <20 CC target — the real "unreviewable in one pass" fix — is met. Optional follow-up: extract the ~12-line 7b glue.)

Recommend merge.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

Copy link
Copy Markdown
Contributor

@vybe vybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated via /validate-pr: verbatim extraction of the #311 access gate (every branch + deny-path ordering preserved), now with net-new isolated coverage (8 characterization tests green pre+post). Clean refactor, green CI, no doc debt. Approving.

@vybe vybe merged commit 50b2cc9 into dev Jun 3, 2026
14 checks passed
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.

2 participants