Skip to content

refactor(tui): adopt command shapes in project group (FEAT-021) - #5717

Open
aboimpinto wants to merge 9 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-021-adopt-command-shapes-in-tui-project-group
Open

refactor(tui): adopt command shapes in project group (FEAT-021)#5717
aboimpinto wants to merge 9 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-021-adopt-command-shapes-in-tui-project-group

Conversation

@aboimpinto

@aboimpinto aboimpinto commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

FEAT-021 converts the TUI project command group (/init, /lsp, /share, /goal — no aliases) to the external command shapes introduced by FEAT-014 and hosted by FEAT-015, following the pattern FEAT-018 (utility) established. The command files remain under codewhale-tui; this PR changes their execution boundary without physically moving them.

Baseline: rebased onto main (20e9574) per maintainer request — the codex/v0912-integration-20260823 base will not land (232 ahead / 324 behind main). The 7-commit series now carries 8 commits directly on main.

This PR:

  • adds the contract-owned CommandProjectContext facet with exact-minimum typed delegates: /lsp status/set, /share session projection (emptiness, length, model, mode label), /goal goal projection (objective, status, pending controls, time, tokens, session-derived effective values) — portable DTOs (ProjectGoalState, ProjectShareProjection, ProjectGoalStatus);
  • converts the complete /project dispatch and render paths to portable contract values — byte-identical output preserved;
  • registers the group via ContextualCommand::from_contract on main's model (no capability bitmask on main yet — the FEAT-019/020 capability machinery lives only on the integration branch): each command is a CommandHandler::Contextual(fn) whose handler destructures exactly its required facets with safe missing-facet errors — /initWORKSPACE only, /lsp → project, /share → project, /goal → project + presentation;
  • routes the cross-group /lsp bridge (config::config::lsp_command) exclusively through the TUI adapter (FEAT-020 D1 pattern); the portable handler never imports the config group;
  • resolves all catalog messages through the presentation facet for /goal (goal_control_accepted, goal_status_idle_hint) and the metadata bridge for all four description keys;
  • removes project from both migration-frontier representations atomically (frontier now config, core, debug, memory, plugins, session, skills — memory/plugins remain pending on main);
  • keeps the temporary TUI-owned CommandResult and the exact AppAction variants the group emits (SendMessage, ShareSession, SetGoalStatus, SetGoalObjective) bounded for FEAT-037.

Tracking: EPIC-006 / FEAT-021 in umbrella Hmbown/CodeWhale#5316.

Dependency boundary

The intended dependency direction remains acyclic:

future codewhale-commands project group
                 |
                 v
codewhale-command-contract ---> codewhale-core / other acyclic leaf crates
                 ^
                 |
codewhale-tui implements the host facets

/project handlers no longer name or call concrete App, config::config, tools::goal, or session-manager code; every host touch crosses the boundary through the CommandProjectContext/WORKSPACE/PRESENTATION adapters. No new crate dependency and no crate move — circular-reference risk is reduced to zero by keeping the contract crate leaf-only and routing cross-group calls exclusively through the TUI adapter.

Dependency-removal tracking (FEAT-037 / FEAT-041): the remaining TUI-owned references in the group are explicitly enumerated for removal:

  • crate::commands::CommandResult and crate::tui::app::AppAction (4 variants) — bounded FEAT-037 temporary references;
  • crate::tools::goal::GoalStatus inside AppAction::SetGoalStatus.status — FEAT-037 must provide a portable goal-status representation for the action payload;
  • crate::utils::{summarize_project, project_tree} and crate::project_context::generate_project_context_pack (leaf, path-parameterized /init helpers) — FEAT-041 must re-home them before the physical move;
  • engine-side share machinery co-located in share.rs (perform_share, upload_gist, render_session_html, write_temp_html, html_escape; crate::dependencies::Gh, tokio/tempfile/chrono) — pre-existing host code, FEAT-041 must extract it host-side.

Review hardening

  • Missing required facets return Command capability unavailable: <facet> — zero .expect() in production dispatch handlers (an improvement over main's current .expect() style in the utility group).
  • Least-capability: /init destructures exactly WORKSPACE (the initial PROJECT|WORKSPACE declaration over-declared and was amended after the critical audit); the other three destructure exactly their required facets.
  • Byte-identical output proven by parity tests with exact-string evidence (LSP status/set messages, goal status/usage/budget/elapsed, share messages, presentation fallback).

Scope and behavior

  • Only groups/project is migrated (four production commands: /init, /lsp, /share, /goal).
  • No command file moves out of codewhale-tui; no shared result/action ownership moves (FEAT-037); CommandResult and AppAction remain bounded temporary data references.
  • No localization system extraction occurs (FEAT-036); the presentation facet maps existing MessageIds.
  • Names, aliases, usage strings, registry order, help/palette discovery, messages, parsing, and safety checks remain unchanged.

Testing (verified locally on this branch)

  • cargo fmt --all -- --check
  • cargo check --workspace --locked — clean
  • cargo test -p codewhale-command-contract --lib --locked — 15 passed
  • cargo test -p codewhale-tui --lib --locked project-scoped — 76 passed; adapter — 4 passed; full TUI lib — 11,362 passed / 0 failed (serial run; excludes documented pre-existing env failures, see below)
  • cargo test -p codewhale-tui --test integration --locked — 277 passed / 0 failed (incl. lifecycle_outbox_exec)
  • cargo test --workspace --locked — PASS, 0 FAILED binaries (excluding documented pre-existing env failures)
  • python3 scripts/test_check_command_crate_boundaries.py — PASS
  • python3 scripts/check-command-migration-manifest.py --baseline-ref origin/main — PASS (project absent from both frontier representations)
  • python3 scripts/test_check_command_migration_manifest.py — PASS

Pre-existing failures verified on pristine origin/main (not caused by this PR):

  • Ghostty/SSH-cap motion tests (config_fancy_animations_keeps_ghostty_full_motion, ghostty_term_program_…, ghostty_term_fallback_…) — environment-sensitive (constrained_frame_rate), fail identically on a clean main checkout in this workspace.
  • runtime_api::* tests — stack overflow in this environment, fail identically on a clean main checkout.
  • clippy -D warnings reports too_many_arguments (9/7) in runtime_threads.rs:2562 — untouched by this PR, fires identically on a clean main checkout with this toolchain; the maintainer CI configuration differs (allow or older clippy).

Checklist

  • Only the project group migrated; zero concrete-App / config::config / tools::goal handler dependencies
  • CommandProjectContext is the only new project-specific host facet
  • Exact per-command facet destructuring: /init = WORKSPACE; /lsp = project; /share = project; /goal = project + presentation
  • Typed signatures and contract-owned result shapes for every operation
  • Existing user-facing output byte-identical (parity tests with exact-string evidence)
  • project removed from both migration-frontier representations
  • /lsp routed through the facet; adapter owns config::config::lsp_command (D3)
  • Missing facets fail safely (Command capability unavailable: <facet>), no .expect() in production dispatch
  • FEAT-037/FEAT-041 dependency-removal obligations enumerated (see above)
  • No manual UI verification required for this structural change

CI status

FULLY GREEN (2026-08-30, head 02e9f34b7): the complete Actions matrix passes — Lint ✅, Safety gate ✅, Test (ubuntu-latest) ✅, Test (macos-latest) ✅, Test (windows-latest) ✅, Change detection ✅, DCO ✅, link ✅, integrations ✅, version drift ✅, npm wrapper smoke ✅, ohos check ✅, GitGuardian ✅. Mergeable ✅.

The earlier macOS/Windows red was main's own sandbox read_guard baseline (failing on main's CI too); the maintainer's fix landed in #5724, and after rebasing onto it the matrix went fully green. The remaining transient reds on intermediate runs (a flaky exec_persistent_service timing test, a macOS runner stall in tools::web/agent_roster) were environmental and cleared on the final run.

No-Issue: FEAT-021 is tracked in umbrella #5316, which must remain open for the remaining decomposition FEATs.

Paulo Aboim Pinto

@aboimpinto
aboimpinto marked this pull request as ready for review August 29, 2026 13:28
@aboimpinto
aboimpinto requested a review from Hmbown as a code owner August 29, 2026 13:28
@Hmbown

Hmbown commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Thank you for this slice — the command-contract facet/handler/test structure is the right shape, and the migration-topology scripts look genuinely useful. Three asks before it can move, in order of importance:

1. The base branch is the blocker. This PR is stacked on codex/v0912-integration-20260823, which is a work-in-progress integration branch that will not land as-is (it is 232 ahead / 324 behind main). As stacked, the PR carries ~239 commits that are not reviewable and the main CI matrix (Lint, Tests on macOS/Ubuntu/Windows, Safety, CodeQL) has never run against it — only 5 checks fired. Please rebase your 7 commits onto main (git rebase --onto main codex/v0912-integration-20260823 feat/FEAT-021-adopt-command-shapes-in-tui-project-group). Happy to help untangle conflicts if any.

2. Scope of the workflow/CI additions — if the rebase surfaces CI-workflow changes beyond the command group, those may be split out; maintainer call after rebase.

3. exec_agent and engine-adjacent changes will get a careful read against the one-turn-loop contract (there is exactly one Engine::run_turn owner; the review will check your changes compose with it rather than parallel it).

Once rebased onto main, CI runs the full matrix and we can review the FEAT-021 command shapes on their merits.

@aboimpinto
aboimpinto force-pushed the feat/FEAT-021-adopt-command-shapes-in-tui-project-group branch from e4b0056 to 40cab37 Compare August 30, 2026 00:38
@aboimpinto
aboimpinto changed the base branch from codex/v0912-integration-20260823 to main August 30, 2026 00:39
@aboimpinto
aboimpinto marked this pull request as draft August 30, 2026 00:39
@aboimpinto

Copy link
Copy Markdown
Contributor Author

Rebased onto main + full matrix babysat — status report

Per your request, the 7 commits are rebased onto main (20e9574) as 8 commits (the exec_agent/outbox commit dropped — main already carries the model field; the lifecycle_outbox_exec integration test passes). The PR now targets main, and the full Actions matrix ran.

Matrix result

Job Result
Lint ✅ SUCCESS
Safety gate ✅ SUCCESS
Test (ubuntu-latest) ✅ SUCCESS — full suite green
Test (macos-latest) ❌ 12 × sandbox::read_guard failures
Test (windows-latest) ⏰ cancelled (job timeout; hang in tui::app::tests)
Change detection / DCO / link / integrations / version drift / npm smoke / GitGuardian ✅ SUCCESS

The remaining red is main's own, not FEAT-021's

  • macOS sandbox::read_guard failures — verified identical on main's own latest CI run (push 20e957406 "fix(sandbox): make the read_guard symlink tests whole-fn unix…", run failing): the same 12 tests fail there. FEAT-021 touches zero sandbox files.
  • Windowsmain's own latest CI run also fails on Windows; our job additionally hit the runner timeout on a tui::app::tests hang (also untouched by FEAT-021).

What this PR changes (rebased shape)

  • CommandProjectContext facet + portable DTOs (ProjectGoalState, ProjectShareProjection, ProjectGoalStatus) on main's model — no CommandCapabilities bitmask (that machinery only exists on the codex/v0912-integration-20260823 branch; main has the FEAT-015 tuple-variant CommandHandler::Contextual(fn) + envelope). Each handler destructures exactly its required facets with safe Command capability unavailable: <facet> errors (an improvement over main's current .expect() style in the utility group).
  • /lsp cross-group bridge (config::config::lsp_command) routed exclusively through the TUI adapter; handlers never name App/config/tools::goal.
  • project removed from both migration-frontier representations (PENDING_GROUPS + topology frontier) — frontier now config, core, debug, memory, plugins, session, skills.
  • FEAT-037/FEAT-041 dependency-removal obligations enumerated in the PR body.

Local verification: workspace compiles; contract 15/15; project group 76/76 + adapter 4/4; TUI lib 11,362/0 (serial, excluding this box's documented pre-existing env failures — ghostty SSH-cap + runtime_api stack overflows, both reproduced on a clean main checkout); integration 277/277; migration/boundary gates PASS. Happy to address the sandbox/Windows baseline once main is green again.

Paulo Aboim Pinto

@aboimpinto

Copy link
Copy Markdown
Contributor Author

Fresh matrix run (head d344960d6) — conclusive result

Job Result
Test (ubuntu-latest) ✅ SUCCESS — full suite green (13,669 tests)
Test (macos-latest) ❌ 12 × sandbox::read_guard failures
Test (windows-latest) ❌ 1 × sandbox::read_guard failure (root_parent_traversal_does_not_escape_above_root)
Lint / Safety gate / Change detection / DCO / link / integrations / version drift / npm smoke / ohos check / GitGuardian ✅ all green

Every failing check is a sandbox::read_guard test in crates/tui/src/sandbox/read_guard.rs — a file this PR does not touch. These same tests fail identically on main's own latest CI run (push 20e957406 "fix(sandbox): make the read_guard symlink tests whole-fn unix, unbreaking Windows CI" — that run is red on macOS and Windows). The failing assertions include root_parent_traversal_does_not_escape_above_root, which suggests the sandbox root-escape guard itself may not hold on macOS/Windows in the current main state — worth a look from the sandbox side.

This PR's code is fully green: Ubuntu runs the complete suite successfully, and the only red jobs are the exact tests main itself is failing. The PR stays in draft until main's sandbox baseline is fixed; once a fix lands on main, a rebase + re-run should go fully green.

Paulo Aboim Pinto

Paulo Aboim Pinto added 8 commits August 30, 2026 10:16
…omes to command contract

- CommandProjectContext with exact-minimum typed delegates (lsp status/set, share projection, goal projection)
- ProjectGoalStatus, ProjectShareProjection, ProjectGoalState portable values
- PROJECT = 1 << 11 capability bit and one optional envelope slot (twelve slots total)
- FakeProject contract tests: object safety, semantic preservation, status variants, bit stability, envelope transport, duplicate-slot rejection, exact D4 capability declarations

Generated with Claude Code
- ProjectAdapter maps App goal/share/LSP state to portable contract values
- Cross-group LSP status/set state owned host-side; workspace path stays on WORKSPACE facet (D2)
- Goal projection carries visible + effective pending-control + session-derived values
- Bundle now owns twelve adapters; contexts()/parts()/constructor wire PROJECT slot
- Adapter tests: LSP state, share projection, goal visible/effective state, exact exposure

Generated with Claude Code
- init.rs: portable init(workspace) via WORKSPACE facet (D2); leaf crate::utils/project_context helpers retained (pure path-parameterized, no App/service state)
- lsp.rs: portable lsp(project) with byte-identical messages; config::config::lsp_command invocation owned host-side by the adapter (D3)
- share.rs: portable share(project) consuming share_projection; ShareSession action preserved; exporter untouched
- goal.rs: portable goal(goal_state, presentation) with goal_control_accepted/goal_status_idle_hint keys; effective-state logic ported; FEAT-037 action payloads bounded
- Each command gets a contextual entry (safe missing-facet errors) + transitional App shell; 17 new tests (71 project-group tests total)
- Adapter lsp_set routes through config::config::lsp_command host-side; key_to_project_message_id added

Generated with Claude Code
… shrink both frontiers

- init/lsp/share/goal implement contract RegisterCommand with exact D4 capabilities; group registers via ContextualCommand::from_contract
- Public dispatch tests: exact capability set per command, public seam dispatch, no-panic matrix (3 tests)
- Remove project from PENDING_GROUPS and topology frontier in the same commit (D7 all-or-nothing); topology group declaration retained (immutable topology)
- feat015 legacy-assertion adds init/lsp/share/goal to MIGRATED
- All gates green: contract 31/31, TUI lib 11421/0, migration 54/54 + live, boundary 8/8 + live, CI wiring 11/11, locale parity, clippy -D warnings

Generated with Claude Code
…on coverage

- format_elapsed_matches_tui_leaf_helper: portable replica byte-identical to crate::elapsed::format_elapsed_secs across 0..3601s
- goal_control_accepted / idle_hint translation tests: translated text, no raw key leak
- goal token fallback tests: session-total fallback (42/100) and engine-count (10/100) paths
- key_to_project_message_id resolves goal runtime keys and rejects unknown
- presentation_translate_resolves_project_keys_with_locale_and_fallback (English fallback preserved)
- All gates green: contract 31/31, TUI lib 11428/0, strict workspace clippy -D warnings clean, migration/boundary/CI/locale gates + diff hygiene

Generated with Claude Code
…o main's tuple-variant model

- /init registers Contextual(init_contextual) destructuring only the
  WORKSPACE facet (D4 amendment: no project-facet method consumed)
- lsp/share/goal register Contextual(<fn>) tuple form (main has no
  CommandCapabilities bitmask); exact facet destructuring + safe
  missing-facet errors express the D4 declaration on main's model
@aboimpinto
aboimpinto force-pushed the feat/FEAT-021-adopt-command-shapes-in-tui-project-group branch from d344960 to 8113aeb Compare August 30, 2026 08:16
@aboimpinto
aboimpinto marked this pull request as ready for review August 30, 2026 10:21
@aboimpinto

Copy link
Copy Markdown
Contributor Author

🟢 Full matrix green — ready for review

Head 02e9f34b7 (rebased onto the sandbox-fixed main, PR #5724):

Job Result
Lint
Safety gate
Test (ubuntu-latest) ✅ 13,995/13,995
Test (macos-latest) ✅ 13,995/13,995
Test (windows-latest)
Change detection / DCO / link / integrations / version drift / npm smoke / ohos check / GitGuardian

Mergeable ✅. No failures anywhere.

The project group now adopts the command shapes on main's model: one CommandProjectContext facet with exact-minimum typed delegates, portable DTOs, byte-identical output, safe missing-facet errors, the /lsp cross-group bridge routed through the TUI adapter, and project removed from both migration frontiers. FEAT-037/FEAT-041 dependency-removal obligations are enumerated in the description.

Paulo Aboim Pinto

@Hmbown Hmbown left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approve with comments — verified against merge-base 8a3f14bf and head 02e9f34b74. Behavior-preserving on every path checked: /lsp output is byte-identical to baseline lsp_command (synonym sets, trim, error strings), the /goal effective-state branching and bare-goal prompt are unchanged, format_elapsed is a parity-tested replica of elapsed::format_elapsed_secs, and all four description keys bridge to the same MessageIds with registry order intact. All three asks from the earlier maintainer comment are resolved: 9 commits cleanly on main, no workflow changes, the exec_agent commit is gone. Checks API confirms the full matrix green on this head; author gate OK (all:aboimpinto).

Two nits, neither blocking:

  1. crates/command-contract/src/facets.rs — the CommandProjectContext doc comment still says /init "declares PROJECT | WORKSPACE (D4)". The merged handler destructures exactly WORKSPACE. Please fix the stale line before FEAT-022 copies it.
  2. crates/tui/src/commands/mod.rs — the project names landed in the FEAT_018_UTILITY constant; consider renaming it (e.g. MIGRATED_GROUPS) so the exemption list stays readable as groups accumulate.

For the record (no action needed): translate(...).unwrap_or_default() in goal.rs would yield an empty message if a key ever failed to resolve — impossible today since both keys are registered and the adapter falls back to English.

The FEAT-037/FEAT-041 dependency-removal obligations (incl. the format_elapsed replica re-home) should carry into those FEATs' specs. Thanks for the careful rebase and the honest CI babysitting reports — the thread made this easy to review.

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