refactor: single member_turn_context chokepoint for rules currency (#8609)

[NicholasRBowers]

Problem / Motivation

Design Review follow-up from PR #7235 (member system prompt for crew DM threads), deferred to #8609 while that PR was at the merge-ready point. The "every member turn runs under current rules" invariant was stitched across five hand-coordinated branches: the lifecycle table in build_message, the fresh-session injection in build_session_context, the chat_runner finally-block re-arm, and the PUT rules handler's mark_needs_reinjection with a hand-built dashboard:{member_slot_key(slug)} key. No single chokepoint owned the invariant, so a future session-lifecycle branch added to build_message could silently skip both the member section and the fail-closed rules gate.

Why it matters

The rules layer is the user's safety boundary for a crew member. A lifecycle branch that bypasses both delivery and the rules gate would run the member with no [PERMANENT RULES] at all, silently — and nothing in the old shape would fail: the coordination lived in a comment table, which no test can pin.

What changed (motivation → approach → change)

Goal: make the invariant live in one function a test can pin, exactly as the issue specifies. Approach: extract the decision, not the actions — each branch keeps its own injection mechanics and consults a shared verdict, keeping the diff behavior-preserving.

New in kiro_crew.members:

• MemberLifecycle — the five session-lifecycle states (FRESH, SLIM_RESUME, WARM_REINJECTION, WARM, MINIMAL), derived by member_lifecycle() from the same inputs build_message already branches on, with delivers_section exposing the delivery half of the verdict.
• member_turn_context(member, lifecycle) — THE decision point: every lifecycle either delivers the current section (whose builder reads rules fail-closed) or runs the standalone rules gate; MINIMAL is the pinned deliberate exception (never a member thread by contract). Fails closed on a non-enum lifecycle (TypeError) — the str mixin makes a bare "warm" compare equal to the member while failing the identity tests, which would otherwise yield the one combination the invariant forbids.
• member_thread_session_alias(slug) — the canonical dashboard:<slot key> alias, replacing the three hand-built f-strings in the members handler (rules-write reinjection flag, thread-open history probe, roster transcript-tail probe).

All delivery branches now route through the chokepoint: the gate and both re-injection sites in build_message test enforce_rules_gate / deliver_section, the build_session_context fresh-session site consults the FRESH verdict, slim_resume is derived from the chokepoint's lifecycle rather than re-encoding the resume predicate, and the chat_runner pending flag reads MemberLifecycle.delivers_section (mode-keyed — the member name resolves later, at the context build).

Zero behavior change: every replaced predicate is equivalent to the original on all reachable input combinations.

Tests

New module test/test_member_turn_context.py:

• Verdict truth table across all five lifecycle states; empty-member always inert; frozen dataclass; delivers_section agrees with the verdict.
• The rules-currency invariant itself: every non-MINIMAL lifecycle takes exactly one of the two mechanisms.
• Exhaustive 16-combination member_lifecycle mapping (including the minimal_context-beats-resumed precedence) and full-enum reachability.
• Fail-closed chokepoint: a non-enum lifecycle (including the str-mixin hazard "warm") raises TypeError.
• Canonical alias shape, round-trip through is_member_session_key, bad-slug refusal.
• AST wiring guards (same style as the SEL offload guard): every _build_member_section call in build_message / build_session_context sits under a deliver_section test, the standalone rules read sits under enforce_rules_gate, the lifecycle is derived from member_lifecycle with the real branch inputs, slim_resume reads the chokepoint's lifecycle, the chat_runner flag routes through delivers_section, and no f-string in the members handler carries a dashboard: constant.

Existing member-prompt suites (~270 tests, early-exit re-arm paths included) pass unchanged — no existing test was edited.

Manual verification

N/A — unit coverage sufficient: the change is a behavior-preserving extraction pinned by the existing behavioral suites plus the new structural guards; no user-visible surface changed.

Related Issues

Closes #8609

Pattern harvest

Rule candidate: review-prompt
Pattern: an invariant coordinated across branches by a comment table instead of a callable chokepoint a test can pin

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A comment-table invariant becomes one test-pinned chokepoint; I verified all five replaced predicates are equivalent on every reachable input, so the extraction is genuinely behavior-preserving.

Suggestions

• Nothing serializes MemberLifecycle — dropping the str mixin (plain Enum) would eliminate the "warm"-compares-equal hazard outright, letting the TypeError guard and its defensive tests shrink instead of defending a hazard the class created.

[DESIGN-REVIEWED] 2d7e06e

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2d7e06e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification is complete. The change is a design-review-deferred, cause-level refactor with every declared item present; the two real findings are zero-consumer internal surface (the MemberTurnContext.member field and the str mixin on the enum, which manufactures the very hazard the guard and a test then defend against). Neither is one-way-door surface, so this lands at CONCERNS with subtractions.

First-Principles-Verdict: CONCERNS

Cause-level fix with a recorded provenance (#8609), but two pieces of the new surface have zero consumers: the member field and the str mixin that creates the very hazard it then guards.

What this change ships

Intent: make "every member turn runs under current rules" enforceable by one testable function instead of a comment table — a FIX (deferred Design Review finding, recorded in issue #8609, so the requirement is derived).

1. Rules gate and both re-injections in build_message now consult one verdict function — justified
2. Fresh-session member injection consults the same verdict — justified
3. slim_resume derived from the lifecycle instead of a second spelling of the predicate — justified
4. Chat-runner re-arm flag derived from the lifecycle's delivery verdict — justified, declared
5. Three hand-built dashboard: member keys replaced by one canonical helper — justified (0 unfixed siblings: grepped dashboard:.*member_slot_key, all remaining f"dashboard:{...}" sites are non-member slot keys)
6. New enum, mapper, frozen verdict dataclass in members.py — justified, 2 real call sites each
7. Non-enum lifecycle now raises TypeError — justified, declared
8. New behavioral + AST-guard test module — justified

Watch

MemberLifecycle(str, Enum): nothing serializes it or compares it to a string (grepped lifecycle.value and lifecycle == across src/: 0 hits) — the mixin's only observable effect is the "warm"-compares-equal hazard that the TypeError guard, a comment block, and test_unrecognized_lifecycle_is_refused_not_failed_open then exist to neutralize.

Subtractions

• Drop the str mixin: class MemberLifecycle(Enum) deletes the pinned hazard; the isinstance refusal can stay as deny-by-default.
• Drop MemberTurnContext.member — zero production reads (grepped \.member in context.py: 0; only tests assert it); callers already hold member.

[FIRST-PRINCIPLES-REVIEWED] 2d7e06e

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9 — this comment is updated in place on each push.

Review details

This is a behavior-preserving refactor consolidating member-lifecycle branching into the member_turn_context chokepoint. I verified each changed site against the code it replaces:

• FRESH leg (build_session_context:2704): member_turn_context(member, FRESH).deliver_section ≡ if member, and it sits below the confirmed minimal_context early-return at line 2572.
• Rules gate (build_message:3161): enforce_rules_gate = member and (lifecycle is WARM) = member and not is_new_session and not needs_reinjection — identical to the old gate, and minimal_context is irrelevant in both (cron carries member="").
• slim_resume (3186): inside if is_new_session:, lifecycle is SLIM_RESUME ⇔ resumed and not minimal_context.
• SLIM_RESUME / WARM_REINJECTION delivery (3290, 3407): both if _member_turn.deliver_section sites are reached only under slim_resume/needs_reinjection branches where the lifecycle delivers, so they collapse to if member.
• chat_runner:6547: slot.mode=="member" and member_lifecycle(is_new, resumed, False, False).delivers_section = slot.mode=="member" and is_new (SLIM_RESUME and FRESH both deliver).
• handlers/members.py: read_dm_binding refuses any binding whose slot_key != member_slot_key(slug) (line 550), and row["slug"] is slug_for_name-validated (bad slugs continued at 143), so member_thread_session_alias(row["slug"]) is byte-equal to the old f"dashboard:{row['slot_key']}" and raises nowhere new. The other two sites are byte-identical substitutions of the same f-string.

The isinstance fail-loud and deny-by-default verdict are sound; no security invariant is weakened, no model literal or harness negation added, no blocking: true rule touched.

No findings.

[OPUS-REVIEWED] 2d7e06e

Verdict parsed from the review's SHA-scoped output markers for commit 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2d7e06e6c1f0ef4f0c9a6645a6b1b2a627afa1e9: <one-sentence reason>

[NicholasRBowers]

Answering the First Principles CONCERNS (advisory; both subtractions acknowledged individually):

str mixin on MemberLifecycle — valid subtraction, deliberately not folded in at the review-ready point: the TypeError guard makes the mixin's hazard unreachable (dropping it changes no behavior), and a cosmetic re-push would re-arm every review lane on the whole diff. Will drop it in a follow-up or on maintainer request before merge.

MemberTurnContext.member — kept deliberately: the verdict is a frozen decision record, and carrying its subject keeps it attributable when logged or inspected without reconstructing the call site; the truth-table tests read it. "Zero production reads" is accurate today — the field is the record's API surface, not plumbing a branch forgot to remove.

[bolichen97]

Tech Lead review: APPROVE.

Verified behavior preservation at every converted predicate against the head source, not just the reviewer summaries:

1. context.py:2704 (FRESH leg) - member_turn_context(member, FRESH).deliver_section reduces to bool(member) because FRESH is in delivers_section, so the hard-coded lifecycle is equivalent for any caller of build_session_context, independent of whether the minimal early-return at 2572 dominates it.
2. context.py:3161 (rules gate) - enforce_rules_gate = member and lifecycle is WARM, and member_lifecycle yields WARM exactly when not is_new_session and not needs_reinjection. Identical to the removed gate.
3. context.py:3186 (slim_resume) - sits inside if is_new_session: (line 3170), where lifecycle is SLIM_RESUME <=> resumed and not minimal_context. Identical inside that branch.
4. context.py:3290 - inside if slim_resume: (3266), so lifecycle is SLIM_RESUME, which delivers; reduces to if member.
5. context.py:3407 - inside if not is_new_session and needs_reinjection: (3365), so lifecycle is WARM_REINJECTION, which delivers; reduces to if member.
6. chat_runner.py:6547 - with minimal_context=False, needs_reinjection=False, delivers_section is True exactly when is_new (FRESH and SLIM_RESUME both deliver, WARM does not), so it equals the removed bool(slot.mode == "member" and is_new).

All three _build_member_section call sites are converted; none were left branching by hand.

member_thread_session_alias extraction is also equivalence-safe: a non-empty row["slot_key"] can only come from read_dm_binding, which rejects any binding whose slot_key is not member_slot_key(slug) (members.py:528), so the substitution yields the same string. It cannot raise a new MemberSlugError either, since read_dm_binding already called member_slot_key(slug) unguarded on the path that produced the row.

CI: 57 success / 6 skipped / 0 failure on the head sha, 0 annotations anywhere (the single Focus Cue annotation is a notice-level report about 5 pre-existing untouched elements). All five AI lanes non-blocking; First Principles CONCERNS is advisory and both subtractions were answered by the author. The new AST wiring test pins the chokepoint so a future delivery branch cannot re-hand-roll the predicate.