refactor(tui): adopt command shapes in session control slice (FEAT-024)

[aboimpinto]

Summary

FEAT-024 converts the TUI session-control slice to portable command shapes while keeping all production files in codewhale-tui:

• /relay (/batonpass, /接力)
• /rename (/gaiming, /chongmingming)
• /resume (/r)
• /rc (/remote-control)
• /remote-env
• /title (/tabtitle, /window-title)

Baseline: current origin/main at 19e460889. The branch is eleven signed commits ahead with no merge commits.

This PR:

• adds contract-owned CommandSessionControlContext and typed session-control DTOs;
• allocates SESSION_CONTROL = 1 << 14 while retaining CommandCapabilities(u16);
• declares exactly SESSION_CONTROL for /relay, /rename, /resume, /rc, and /title;
• declares exactly SESSION_CONTROL | PRESENTATION for /remote-env;
• exposes only declared facets through the restricted command envelope;
• keeps concrete App, session manager, picker, persistence, Git-origin, browser, remote-service, checkpoint, and Work-state machinery in SessionControlAdapter;
• keeps /rc open synchronous through the host facet, preserving existing browser behavior;
• returns typed relay projections and formats plan-step status labels in the portable handler;
• registers all six handlers through the public contextual dispatch seam;
• retains the root session migration frontier for FEAT-025 and FEAT-026;
• keeps TUI-owned persistence, Git, browser, remote-service, and recovery regressions outside the future movable groups/session directory.

Tracking: EPIC-006 / FEAT-024 in umbrella #5316.

Dependency boundary

Portable session-control handlers do not access concrete App, session managers, pickers, persistence services, Git wrappers, browser helpers, prompt/TODO/plan/Work-state types, configuration, or host services. Missing authority fails closed with:

Command capability unavailable: session_control

/remote-env also fails safely when its separately declared presentation facet is unavailable.

The only temporary TUI-owned result/action references are the exact compatibility payloads assigned to FEAT-037:

• CommandResult;
• AppAction::SendMessage(String);
• AppAction::LoadSession(PathBuf);
• AppAction::RemoteControl(Start | Stop) and RemoteControlAction;
• AppAction::OpenExternalUrl { url, label }.

The remaining staged dependencies are explicit:

• FEAT-016 owns registry extraction;
• FEAT-036 owns localization ownership;
• FEAT-037 owns the shared result/action data above;
• FEAT-025 and FEAT-026 own the remaining session slices;
• FEAT-043 owns the physical session-group move and keeps host regressions TUI-side.

No new crate dependency and no physical production-file move occur here. codewhale-command-contract remains independent of codewhale-tui.

Behavior and scope

• Structural migration only: command names, aliases, registry order, parsing, check order, messages, actions, persistence behavior, browser timing, and output remain unchanged.
• /relay composes the existing byte-identical relay instruction from typed semantic data.
• /rename and /title preserve checkpoint recovery, metadata and Work-state persistence, terminal-control sanitization, and existing length boundaries.
• /remote-env preserves HTTPS/SSH/CNB origin normalization, credential safety, branch encoding, malformed-origin rejection, detached-HEAD behavior, and localized placeholders.
• /resume retains transition-gate precedence and exact host call order.
• No localization extraction, shared outcome move, registry extraction, session export/structcopy migration, or physical group move is included.
• No Gherkin expectations changed.

Review remediation included

A whole-feature critical review found and this branch resolves the following before PR creation:

• restored the complete TUI-host regression surface through public dispatch;
• removed duplicated rename production logic and made the adapter/public-dispatch path authoritative;
• replaced adapter-rendered PlanStep.status_label: String with contract-owned PlanStepStatus;
• added exact restricted-envelope coverage for control-only, unrelated, and control-plus-presentation capabilities;
• added missing-presentation coverage for /remote-env;
• added exact fake-facet operation counts, transition-gate precedence, and call-order assertions;
• recorded FEAT-024's exact temporary payload handoff in FEAT-037.

Final internal review verdict: APPROVED.

Owner-review follow-up

The feedback reviewed at d8e43fe1d is resolved in d4b68963d:

• /rename now sanitizes and enforces the shared 100-character policy in the portable handler before any host mutation; /rename, /title, and the host regression suite use one MAX_TITLE_LEN;
• TitleSetOutcome and its unreachable mismatch arms are removed; explicit set_window_title and clear_window_title delegates return only host-stage errors;
• remote_env module visibility is private again;
• the duplicated six-module test message helper is consolidated in control_test_support.rs;
• /title manager/session error precedence is restored to the exact baseline order noted in review.

Testing

All commands below passed on the final rebased head:

• cargo fmt --all -- --check
• cargo test -p codewhale-command-contract --locked — 52 passed
• cargo test -p codewhale-tui --tests --locked -- --test-threads=1 — TUI library 11,784 passed / 13 ignored; Cucumber 16 passed; integration 289 passed
• cargo test --workspace --all-targets --all-features --locked -- --test-threads=1 — complete workspace matrix passed; all-feature TUI library 11,799 passed / 13 ignored; Cucumber 33 passed; integration 289 passed
• cargo build --release --workspace --locked
• strict Clippy for codewhale-command-contract and codewhale-tui with --all-targets --all-features --locked -D warnings
• session group — 88 passed
• restored public-dispatch control regressions — 18 passed
• control adapter/public-dispatch coverage — 11 passed
• migration fixtures — 56 passed; frontier remains [config, core, debug, session]
• command-boundary fixtures — 8 passed
• CI wiring tests — 11 passed
• locale parity — all 15 non-English locales match the 2,087-key reference
• git diff --check

The TUI and workspace runs used RUST_MIN_STACK=8388608, --test-threads=1, and removed the harness TMUX marker so terminal-policy tests observed their intended fixtures.

Checklist

• This PR adds a new layer/module/abstraction — CommandSessionControlContext replaces direct host access for exactly six session-control handlers
• Updated docs or comments as needed
• Added or updated tests where relevant
• Verified TUI behavior manually if UI changes — N/A; this is an internal structural refactor with unchanged Gherkin expectations and public-dispatch regression coverage
• All eleven commits carry DCO Signed-off-by trailers
• Harvested/co-authored credit uses a GitHub numeric noreply address — N/A; no harvested/co-authored commits

No-Issue: FEAT-024 is tracked in umbrella issue #5316, which remains open for the remaining EPIC-006 work.

Paulo Aboim Pinto

[Hmbown]

Reviewed at d8e43fe1d, specifically against the shape #5902 (FEAT-023) and #5865 (FEAT-020) established, since you framed this as the next slice in that series. It is a faithful continuation — I could not find an envelope, capability, adapter, registration, or dependency-boundary divergence. Three small things I'd like fixed, one of which partially defeats the slice's own purpose.

Tests I ran

Detached worktree at d8e43fe1d, RUST_MIN_STACK=16777216:

Command	Result
cargo test -p codewhale-command-contract --lib	52 passed; 0 failed
cargo test -p codewhale-tui --lib -- commands::groups::session commands::session_control_regression_tests	89 passed; 0 failed
cargo test -p codewhale-tui --lib -- commands::contract	55 passed; 0 failed

Check Signed-off-by is green and I confirmed it directly: all ten commits carry Signed-off-by: Paulo Aboim Pinto, no merge commits. Thank you — that is the third series in a row where I have had nothing to say about the trailers.

Shape parity — what I checked rather than assumed

• Facet: CommandSessionControlContext at facets.rs:1250-1372 — same file, same trailing-append position, same "synchronous operation-granular delegates + portable receipt structs" style as CommandSessionLifecycleContext (facets.rs:1044+).
• Capability bit: handler.rs:47 SESSION_CONTROL = 1 << 14, the next free bit after SESSION_LIFECYCLE, with the same contexts/ContextParts/with_control wiring. The comment explicitly records that the u16 backing stays per the maintainer resolution on #5902 rather than speculatively widening — exactly the right call, and worth having written down.
• Adapter: SessionControlAdapter<'a> { host: SharedCommandHost<'a> } (contract.rs:1020), into CommandContextBundle (contract.rs:3947), gated in contexts() (contract.rs:3996). Same wiring as SessionLifecycleAdapter.
• Fail-closed: every *_contextual returns Command capability unavailable: session_control before touching anything, matching branch.rs:46's session_lifecycle form. /remote-env checks control then presentation, and its test asserts no control call happens when presentation is missing.
• Localization: your "FEAT-036 owns localization" claim checks out against precedent. #5902's branch.rs emits raw English literals throughout, and only surfaces that already called tr() on origin/main were routed through PresentationAdapter::translate. /remote-env was the only one of the six that did, so it is the only one declaring PRESENTATION. key_to_session_message_id (contract.rs:1750) is the key_to_plugin_message_id pattern from #5865. Placeholder sets on the five CmdRemoteEnv* ids match en.json. No defect here.
• Duplicated logic: normalize_repo_slug, read_git_value, hosted_work_url, valid_branch_name, split_scp_origin, resolve_target, import_session_container, import_foreign each have exactly one definition crate-wide, all now in contract.rs. The originals were deleted, not left behind. That is the "migrate the last consumer or do not start" rule actually honored.
• Silent failure: I diffed every added let _ =, .ok(), unwrap_or*. contract.rs:1319's open_url(&url).is_ok() discards the error, but origin/main:remote_control.rs:53-58 already did Err(_) =>; relay.rs's let _ = writeln!(..) into a String is infallible and byte-identical to baseline. Nothing new is swallowed.
• Data loss: /rename and /title preserve the #5430 first-snapshot recovery (contract.rs:1152-1158, 1248-1254) and keep the update_session(..) re-sync before persisting, so an in-flight conversation is not overwritten by a stale document; publication failure still returns the post-save partial-success message. /resume's route ordering matches baseline including ||/&& precedence, and transition_blocked() runs before any picker or I/O.

The one I'd like fixed before merge

/rename's length policy crossed the boundary; /title's did not — and the constant went from two copies to three.

origin/main had MAX_TITLE_LEN declared twice, in the two handlers that enforce it:

origin/main:crates/tui/src/commands/groups/session/rename.rs:10
origin/main:crates/tui/src/commands/groups/session/title.rs:17

After this PR it is declared three times, and the two commands enforce it on opposite sides of the seam:

• crates/tui/src/commands/contract.rs:1130-1131 — /rename's check now lives inside the adapter, in the TUI.
• crates/tui/src/commands/groups/session/title.rs:79-80 — /title's check stayed in the portable handler.
• crates/tui/src/commands/session_control_regression_tests.rs:21 — a third copy for the tests.

And contract.rs:1362 documents itself as:

/// Session-name/window-title length policy shared by `/rename` and `/title`.
const MAX_TITLE_LEN: usize = 100;

It is not shared — /title never reads it. A comment that is false on the day it is written is worse than no comment.

The practical consequence is the one this slice exists to prevent: when FEAT-043 moves groups/session out, /title takes its user-facing limit along and /rename leaves its behind in codewhale-tui. Every prior slice put validation and exact message composition in the handler and let the facet return only host-stage errors (branch_portable in #5902 is the reference). Please move the /rename check back into rename_portable and let both handlers share one constant — behavior is already identical (I diffed the ordering: sanitize→length→session_id for rename, length→sanitize for title, both preserved), so this is a pure relocation.

Two small ones

TitleSetOutcome forces two unreachable arms with a new untranslated string. title.rs:73 and title.rs:88 both return CommandResult::error("internal title-state error"). The variant is fully determined by the argument the handler just passed one line earlier (None → Cleared, Some → Set), so the enum encodes a state the caller already knows, and the cost is a new user-visible English string with no baseline counterpart — the only "internal … error" in the migrated session group. Either have the delegate return Result<Option<String>, String> directly, or split it into clear_window_title / set_window_title. Then the arms disappear rather than being papered over.

Unnecessary visibility widening. groups/session/mod.rs:14 changes mod remote_env; to pub(crate) mod remote_env;. The only remote_env:: reference crate-wide is mod.rs:85, in the same parent module, where private mod is sufficient — I grepped. Every sibling in that file is private, and #5902 kept all of its migrated modules private. Revert it.

Nits, take or leave

• fn message(result: &super::CommandResult) -> &str (strip the "Error: " prefix) is duplicated verbatim in all six handler test modules — rename.rs, title.rs, relay.rs, resume.rs, remote_control.rs, remote_env.rs. control_test_support.rs already exists as the shared home for exactly this.
• Test layout diverges from the slice immediately before it: #5902 collected portable-handler tests in one lifecycle_portable_tests.rs; this one puts them inline per handler and adds a shared control_test_support.rs. Inline Fake* is the dominant convention elsewhere in the crate, so I do not think it is wrong — but FakeControl vs CannedLifecycle means the two adjacent slices now read differently for no reason.
• /title error precedence: baseline resolved the SessionManager before checking current_session_id; the adapter checks current_session_id first (contract.rs:1235-1245). Reachable only when the sessions directory is unopenable and there is no active session, and both paths still error — noting it only because your PR body claims exact check-order preservation.
• 15 of 16 capability bits are now allocated. FEAT-025 takes the last one. Whoever picks up FEAT-026 will be widening the type, so it is worth deciding that deliberately rather than under deadline.

One standing risk, not yours

/resume still replaces app.api_messages with the imported transcript without first saving the live conversation (contract.rs:1461). That is unchanged baseline behavior (origin/main:resume.rs:115) and I am explicitly not counting it against this PR — flagging it because this slice is the last time anyone will read that code closely before it moves.

Excellent work, as with the previous three. The dependency-boundary discipline is real and I checked it rather than taking the PR body's word for it.

[aboimpinto]

Thank you for the detailed review and for checking the boundary rather than accepting the PR description at face value. I addressed all three requested changes in d4b68963d:

• /rename now sanitizes and applies the shared 100-character policy in rename_portable before any host mutation. /rename, /title, and the retained host regression suite now use one MAX_TITLE_LEN.
• TitleSetOutcome and the two unreachable internal title-state error branches are gone. The facet now has explicit set_window_title and clear_window_title operations that return only host-stage errors.
• remote_env is private again.

I also took two of the optional cleanup notes while the same code was open:

• consolidated the six identical test message helpers into control_test_support.rs;
• restored /title manager resolution before active-session lookup, matching the baseline error precedence you identified.

I left the inline per-handler test layout unchanged because it remains the dominant local convention and does not affect extraction. The unchanged /resume live-conversation risk remains outside this structural PR.

Focused verification on the new head is green:

• command contract: 52/52;
• session group: 88/88;
• retained session-control regressions: 18/18;
• command adapter/contract suite: 66/66;
• strict command-contract + TUI Clippy with all targets/features;
• migration 56/56, boundaries 8/8, CI wiring 11/11, locale parity, formatting, and diff hygiene.

The PR remains draft while the refreshed complete GitHub matrix runs.

Paulo Aboim Pinto

[Hmbown]

Merged with the matrix and Buildkite green on the exact head. The three nits in the review (MAX_TITLE_LEN now declared three times with contract.rs claiming it is shared by /title, the two untranslated unreachable TitleSetOutcome arms, and the unnecessary pub(crate) mod remote_env) are welcome as a small follow-up in the same series. Thanks for FEAT-024.

[aboimpinto]

The refreshed matrix is green on the exact feedback-fix head d4b68963d:

• Linux, macOS, and Windows tests;
• lint, Safety gate, integrations, and version drift;
• DCO and contribution gates;
• OpenHarmony cargo check;
• npm wrapper and web checks;
• GitGuardian;
• Buildkite codewhale-ci.

All requested review findings are addressed, the branch is mergeable, and the PR is now marked ready for owner review.

Paulo Aboim Pinto

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.