Skip to content

refactor(tui): adopt command shapes in skills group (FEAT-022) - #5825

Closed
aboimpinto wants to merge 8 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-022-adopt-command-shapes-in-tui-skills-group
Closed

refactor(tui): adopt command shapes in skills group (FEAT-022)#5825
aboimpinto wants to merge 8 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-022-adopt-command-shapes-in-tui-skills-group

Conversation

@aboimpinto

@aboimpinto aboimpinto commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

FEAT-022 converts the complete TUI skills command group to the portable command shapes while keeping the files in codewhale-tui:

  • /skills (jinengliebiao)
  • /skill (jineng)
  • /review (shencha)
  • /restore (no aliases)

Baseline: rebased directly onto current origin/main (d7042ab83) on 2026-09-02. This branch does not depend on the abandoned v0.9.12 integration branch; the current main frontier still includes memory/plugins.

This PR:

  • adds the contract-owned CommandSkillGroupContext facet and portable DTOs for discovery, activation, mutation receipts, remote registry/suggest/sync, review, snapshot listing/restoration, and approval state;
  • adds the TUI-owned SkillGroupAdapter, which keeps concrete App, skills/plugin services, network/config, SnapshotRepo, history mutation, and async bridging behind the contract boundary;
  • converts all four handlers to CommandHandler::Contextual with safe missing-facet errors;
  • uses exact handler dependencies on main's no-bitmask model:
    • /skills = skill-group facet;
    • /skill = skill-group + shared SKILLS cache-refresh facet;
    • /review = skill-group facet only;
    • /restore = skill-group facet only;
  • preserves the baseline cache policy with exact call-count tests: install/update/uninstall refresh once, trust and /review refresh zero times;
  • registers all four commands through ContextualCommand::from_contract and removes skills from both migration-frontier representations;
  • extends the migration gate with fail-closed tracking for retained dispatcher-only host machinery pending FEAT-042.

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

Dependency boundary

Portable handlers no longer name concrete App, plugin/skill services, network/config services, SnapshotRepo, or history cells. The TUI adapter owns those operations and returns contract-owned values.

The intentionally retained host-only dispatcher path remains co-located for now:

  • run_skill_by_name
  • discover_visible_skills
  • activate_skill_with_task
  • activate_skill

These functions remain TUI-side and are tracked fail-closed for extraction to a host module in FEAT-042 before the skills group physically moves.

Temporary data-only references remain assigned to FEAT-037:

  • CommandResult
  • AppAction::SendMessage(String)
  • AppAction::OpenSkillsManager

No new crate dependency and no crate move occur in this PR. codewhale-command-contract retains no codewhale-tui dependency.

Behavior and scope

  • Structural migration only; command names, aliases, usage, registry order, parsing, messages, actions, and trust/network behavior remain unchanged.
  • /review intentionally does not refresh the skill cache; the baseline performed zero refreshes. A post-implementation audit removed a synthetic SKILLS dependency and added regression coverage.
  • No localization extraction (FEAT-036), outcome ownership move (FEAT-037), or physical group move (FEAT-042).
  • No Gherkin expectations changed.

Review notes

  • crates/tui/src/skills/system.rs: removes the now-dead TUI BundledSkillTier::heading; the only consumer moved to the portable contract enum.
  • Two minimal Boy Scout Clippy allows are included on pre-existing too_many_arguments signatures in runtime_threads.rs and tui/underwater.rs. The strict gate was red at the pristine branch baseline with the active toolchain; no signatures or behavior were changed.

Verification after rebase

  • cargo fmt --all -- --check
  • python3 scripts/check-command-migration-manifest.py --baseline-ref origin/main
  • python3 scripts/test_check_command_migration_manifest.py — 56 passed
  • python3 scripts/check-command-crate-boundaries.py
  • cargo test -p codewhale-command-contract --locked — 29 passed
  • focused TUI skills tests — 39 passed
  • FEAT-022 public-dispatch tests — 4 passed
  • cargo test -p codewhale-tui --test cucumber --locked — 16 passed; unchanged scenarios
  • targeted command-contract/TUI Clippy with -D warnings
  • git diff --check

The implementation checkpoint also completed the configured full workspace/test/release gates before the final rebase. Upstream CI is fully green at head c823cebee: Linux, macOS, Windows, lint, safety, integrations, version drift, OHOS, npm/mobile smoke, link/signoff, and security checks all passed; conditional jobs skipped as expected.

Checklist

  • Exactly four skills commands migrated
  • Portable handler logic has no direct concrete App/TUI-service dependency
  • /review uses only the group facet and performs zero cache refreshes
  • /skill mutation refresh counts preserve the baseline exactly
  • Missing facets fail safely without panic
  • skills removed from both migration frontiers
  • FEAT-037 action ownership and FEAT-042 host extraction handoffs recorded
  • Gherkin expectations unchanged and integration suite green locally
  • Complete upstream CI matrix green

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

Paulo Aboim Pinto

Paulo Aboim Pinto added 8 commits September 2, 2026 11:03
…slot to command contract

- CommandSkillGroupContext trait with exact-minimum typed delegates (D1)
- Portable values: SkillRegistryProjection, SkillEntry, mutation receipts,
  remote registry/suggest/sync outcomes, activation/review outcomes, snapshots
- skill_group envelope slot (eleven total) + with_skill_group builder (D4)
- Shared CommandSkillsContext unchanged (D2 regression test)
- 13 new contract tests: object-safety, variant closure, slot behavior

Generated with Claude Code
…osure

- SkillGroupAdapter implements CommandSkillGroupContext: discovery projection,
  activation with plugin-authority verification + side effects, synchronous
  mutation receipts, remote registry/suggest/sync, review, snapshots, approval
- Bundle gains the eleventh adapter slot; 'ten facets' comment updated (D3)
- Contract refinements locked during implementation: SkillSyncOutcome policy
  variants; ReviewOutcome::Ready payload dropped (side effects host-side)
- 12 adapter tests + 2 mapping unit tests; TUI lib suite green

Generated with Claude Code
- /skills and /skill handlers consume CommandSkillGroupContext (+ shared SKILLS
  for /skill per D4): discovery projection, remote/suggest/sync, receipts,
  activation, safe missing-facet errors; byte-identical output
- Cache-refresh policy moved to the portable layer via SKILLS.refresh_skill_cache
  (D2); /skill new alias is handler-side parsing; adapter delegates simplified
- Dead TUI BundledSkillTier::heading removed (its only consumer was the migrated
  listing; heading now lives on the portable SkillBundledTier)
- Transitional shell execute() delegates to contextual dispatch (Phase 6
  replaces it with the contract bridge); run_skill_by_name + host machinery
  retained co-located for FEAT-042
- 25 fake-facet parity tests

Generated with Claude Code
…atch

- /review destructures group + SKILLS (D4); host performs discovery + side
  effects via run_review; handler renders exact not-found error and SendMessage
- /restore destructures group only (no MODE_POLICY, D4); host owns SnapshotRepo
  + approval state; handler owns parsing, trust gate, and exact listing text
- Transitional shell execute() delegates to contextual dispatch (Phase 6)
- 12 fake-facet parity tests (exact strings, trust gate, host-error boundary)

Generated with Claude Code
…ink both frontiers

- skills/mod.rs registers all four commands via ContextualCommand::from_contract;
  legacy FunctionCommand registrations removed (metadata now contract CommandInfo
  with description_key; transitional shells removed)
- skills removed from PENDING_GROUPS and topology frontier in the same change (D8);
  MIGRATED_GROUPS gains skills/skill/review/restore
- Migration gate extended with RETAINED_HOST_MACHINERY (fail-closed FEAT-042
  tracking for run_skill_by_name/activate_skill_with_task/activate_skill that keep
  &mut App by design); fixtures updated
- 3 public-dispatch/registration tests; all gates green (migration fixtures + live,
  boundary fixtures + live, CI wiring, locale parity)

Generated with Claude Code
…tion for empty-handler groups

- 2 hermetic fixture tests: retained-host exemption and signature-loss detection
- retained-host validation now runs before the empty-handlers early exit so a
  group whose retained machinery all lost its App signature still fails closed

Generated with Claude Code
- feat022_aliases_dispatch_through_public_seam: jinengliebiao / jineng / shencha
  resolve through the registry to the portable handlers
- Collapse nested if in /skills inspect path (clippy collapsible_if)
- Boy Scout: allow(clippy::too_many_arguments) on two pre-existing baseline
  signatures (runtime_threads::accumulate_runtime_cost_coverage,
  underwater::render_launch_composer); strict clippy gate was RED at the pristine
  baseline f76252d on this toolchain
- Full gate set green: clippy -D warnings workspace-wide, TUI --tests 277/277,
  contract 29/29, all Python gates

Generated with Claude Code
Remove the synthetic SKILLS dependency and cache refresh from /review because the baseline performs neither. Keep /skill mutation refresh behavior unchanged and add exact call-count coverage for install, update, uninstall, and trust.

Verified with focused contract/TUI tests, public dispatch tests, migration gates, the full default Cucumber suite, and targeted clippy -D warnings.
@aboimpinto
aboimpinto marked this pull request as ready for review September 2, 2026 10:21
@aboimpinto
aboimpinto requested a review from Hmbown as a code owner September 2, 2026 10:21

@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.

Reviewed the diff against tonight's 0.9.12 integration (integration/shell-brand-20260902, PR #5826 — merging first as the release wave). Notes:

  • Merge order: land this after #5826. Your branch is based on current main (good), but #5826 rewrites two files you touch:
    • crates/tui/src/tui/underwater.rs — your two changes (moving the COMPLETION_BREATH_MS import up, and #[allow(clippy::too_many_arguments)] on render_launch_composer) will re-apply trivially, and the allow may not even be needed post-rebase since the workspace clippy gate carries -A clippy::too_many_arguments.
    • crates/tui/src/commands/groups/skills/* — no conflict expected; the mega PR does not touch the skills group.
  • Vocabulary: this PR's copy says Fleet/fleet correctly (nothing to fix — the 2026-09-01-evening founder decision makes fleet the public term and /pod a compatibility alias, which #5826 sweeps everywhere else).
  • The command-migration manifest update (scripts/command-migration-topology.json) and the cache-refresh call-count tests look like the right shape for the no-bitmask dependency model.

Nothing blocking from this review — rebase after #5826 and the suite decides the rest.

@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.

Reviewed the diff against tonight's 0.9.12 integration (integration/shell-brand-20260902, PR #5826 - merging first as the release wave). Notes:

  • Merge order: land this after #5826. Your branch is based on current main (good), but #5826 rewrites two files you touch:
    • crates/tui/src/tui/underwater.rs - your two changes (moving the COMPLETION_BREATH_MS import up, and the clippy allow on render_launch_composer) will re-apply trivially, and the allow may not even be needed post-rebase since the workspace clippy gate carries -A clippy::too_many_arguments.
    • crates/tui/src/commands/groups/skills/* - no conflict expected; the mega PR does not touch the skills group.
  • Vocabulary: this PR's copy says Fleet/fleet correctly (nothing to fix - the 2026-09-01-evening founder decision makes fleet the public term and /pod a compatibility alias, which #5826 sweeps everywhere else).
  • The command-migration manifest update (scripts/command-migration-topology.json) and the cache-refresh call-count tests look like the right shape for the no-bitmask dependency model.

Nothing blocking from this review - rebase after #5826 and the suite decides the rest.

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