Skip to content

feat(commands): adopt capability shapes in the TUI session-export slice (FEAT-025) - #6096

Open
aboimpinto wants to merge 3 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice
Open

feat(commands): adopt capability shapes in the TUI session-export slice (FEAT-025)#6096
aboimpinto wants to merge 3 commits into
Hmbown:mainfrom
aboimpinto:feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice

Conversation

@aboimpinto

@aboimpinto aboimpinto commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

FEAT-025: adopt command shapes in the TUI session-export slice

Rewires /export (alias /daochu) through the portable command contract while the command file
stays in codewhale-tui. Structural migration only: no user-visible behaviour change.

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

This is one bounded slice of EPIC-006, which extracts the TUI command system into
codewhale-commands in two passes: adopt capability shapes in place, group by group, then move the
already-decoupled code. This PR does the first pass for session export only.

What changes

Contract (codewhale-command-contract)

  • New CommandSessionExportContext: seven operation-granular, &self delegates returning
    contract-owned DTOs (metadata, transcript projection, semantic restore points, turn handoff,
    clipboard mode/recovery/delivery, destination resolution/writing).
  • New SESSION_EXPORT capability, declared by /export and nothing else. The TUI bridge exposes only
    that facet.
  • D9 data minimisation at the type level: internal reasoning bodies, reasoning signatures, and
    inline/local image payloads are replaced by typed omission markers while the projection is built,
    so they never enter the DTOs.
  • The raw-bits accessor is #[cfg(test)]; a canary pins the fact that bits 0-15 are now fully
    allocated (see "Reviewer notes").

TUI (codewhale-tui)

  • SessionExportAdapter is the sole owner of concrete export machinery: metadata derivation,
    authoritative/visible-history projection, semantic restore-point projection, the existing
    turn_handoff_markdown renderer, clipboard mode/recovery/delivery, and protected destination
    resolution/writing.
  • Host services that must outlive the physical move live in commands/session_export_host.rs,
    outside groups/session: the shared last-copy.md recovery writer (now reused by /copy) and
    the protected export-destination resolver/writer. Algorithms, check order, error text and platform
    behaviour are the baseline implementations relocated unchanged.
  • The portable handler owns parsing, document rendering, redaction, restore-point correlation,
    operation sequencing and every user-visible string. It renders from an owned projection and redacts
    JSON in place, so no payload is copied twice.
  • /copy and /structcopy only rewire their helper imports; neither is migrated.

One shared sanitizer. The pure ANSI/URL/text/secret-redaction primitives move to
codewhale-secrets (redact + sanitize) so portable command helpers have one acyclic
implementation with no TUI, client, or config dependency. Bodies are relocated verbatim;
config::persistence re-exports to keep its public API stable, and tui::client / tui::osc8
delegate so their callers cannot drift.

Preservation evidence

Behaviour is preserved, and this is proven rather than asserted.

  • Four baseline-captured goldens. Captured from the pre-change implementation in a scratch
    worktree by dispatching frozen fixtures through the baseline public /export seam: authoritative
    transcript with no restore repository (header, every content variant, omission markers, redaction,
    ANSI stripping); the turn handoff; the visible-history fallback over a workspace with recorded
    restore points; and correlation against recorded restore points (table, newest-first order, tool
    snapshots listed but not correlated, the ambiguous double match, the no-match line). Only genuinely
    clock-derived fields are normalised, with a unit test pinning the normaliser so it cannot quietly
    widen. Everything else is compared byte-for-byte. Re-capturing them from the implementation under
    test would make the comparison a tautology; the fixture header says so.
  • Verbatim relocations, checked by hand: redact.rs is a byte-identical move of the 586-line
    block from config::persistence; strip_ansi_impl/utf8_seq_len are byte-identical to the former
    osc8 bodies; redact_url_for_display is identical apart from reqwest::Urlurl::Url (the
    same type); the parser and the protected-file/recovery writers are literal copies.
  • Exact behaviour retained: read-only snapshot access (never creates a repository), overwrite
    refusal (and --force), private permissions, symlink rejection, unprotected-path errors,
    clipboard→recovery ordering, and all usage-error text.

Least authority and extraction readiness

  • /export declares exactly SESSION_EXPORT; no PRESENTATION facet, no new catalog string.
  • A missing facet returns Command capability unavailable: session_export; there is no .expect()
    or panic on the facet path.
  • A shared test helper destructures all sixteen ContextParts slots, so "exposes no unrelated
    facet" is a compile-time guarantee: a new facet fails to compile until it is classified, and every
    slot is asserted absent in one place.
  • No App, clipboard, snapshot, filesystem, history, or turn-handoff type crosses the facet.
  • Below 1 << 15 the backing storage stays u16 and NONE semantics are unchanged.

Extraction proven by compilation. The handler plus its 21 portable tests were compiled in a
throwaway crate outside the TUI, with only a stand-in for the temporary FEAT-037 CommandResult.
Library and test targets build and all 21 tests pass with no codewhale-tui in the dependency
graph
. The only adjustment needed was making the stand-in faithful to CommandResult::error
(Error: {msg} prefix) - which is the point: that outcome type is the one TUI-owned thing the slice
touches. Host-bound tests and fixtures stay at the commands root, outside the group that will move.

Post-implementation audit

The slice was audited against the pristine pre-change tree before this PR, and again after the audit
fixes. No functional defect was found. The findings that changed the code are folded into the commits
above; the ones that did not are recorded here rather than silently dropped:

Finding Disposition
Complete-document parity was initially proven for only one state (authoritative transcript + no repository) Fixed: the four goldens above cover the remaining states
"No unrelated facet" was a spot-check of six of sixteen slots Fixed: exhaustive destructuring helper
Correlation compared the rendered role string against "user" Fixed: is_user_role carries the exact Role::User comparison, so Role::Unrecognized("user") is not treated as a user turn
Stale export::<helper> seam comments in /structcopy Fixed, and the audits now reject a stale reference
Extraction audits were substring scans; sanitizer purity was a manifest text match Fixed: token-aware scans with their own unit test, and the purity check moved into the boundary gate as a real dependency-graph check
A relocated test dropped the repository's secret-scanner avoidance convention: the pre-move private-key test assembled its PEM header at runtime so no scanner could match it, and the moved version used a literal header (GitGuardian flagged 1 secret) Fixed: restored the runtime assembly idiom rather than adding a scanner ignore; GitGuardian now scans the commits clean
The adapter's Unreadable restore arm has no adapter-level test Not fixed, recorded. It is reachable only when the git binary cannot be resolved (SnapshotRepo::list maps a failing git log to an empty list), and dependencies::Git::resolve is process-cached, so any test would be flaky. Behaviour is unchanged from the baseline.
Peak memory: the projection is an owned copy of the transcript Accepted. The facet must return owned data because SharedCommandHost lends App through a RefCell, and a dyn facet cannot return data borrowed from a temporary Ref. Removing it needs a host proxy that can lend a borrowed projection; the avoidable second copy was removed instead.

Scope disclosure

crates/tui/src/tui/clipboard.rs carries an unrelated, test-only bounded retry for a fork/exec
ETXTBSY race in the wl-paste regression. It is kept because the configured workspace gate was
intermittently red without it, and it is disclosed rather than hidden. There is no equivalent change
on main to reconcile against. Happy to split it into its own PR on request.

Not fixed here

  • CommandCapabilities widening to u32 (see reviewer notes) - owned by the next slice.
  • A borrowed/Cow export projection, which needs the host-proxy change described above.
  • No scope expansion: FEAT-037's temporary allowance is still exactly CommandResult; no AppAction,
    callback, or deferred action was added. The root session migration frontier remains pending.

Verification

Rebased onto current main; all gates re-run at the rebased head:

Gate Command Result
Format cargo fmt --all -- --check PASS
Clippy cargo clippy --workspace --all-targets --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or PASS — 0 warnings
Tests sh scripts/with-hermetic-test-home.sh cargo nextest run --workspace --all-features --locked --profile ci PASS — 15403 run / 15403 passed / 21 skipped
Doctests sh scripts/with-hermetic-test-home.sh cargo test --workspace --all-features --locked --doc PASS
Release build cargo build --release --locked -p codewhale-cli -p codewhale-tui PASS
Contract tests cargo test -p codewhale-command-contract --lib --locked PASS — 59 / 59
Migration gate python3 scripts/test_check_command_migration_manifest.py + live check PASS — frontier unchanged: [config, core, debug, session]
Boundary gate python3 scripts/test_check_command_crate_boundaries.py + live check PASS — no TUI edge from codewhale-command-contract or codewhale-secrets
CI wiring / locale parity python3 scripts/test_ci_migration_wiring.py / python3 scripts/check-tui-locale-parity.py PASS / 2211 of 2211 keys per pack
Doc build cargo doc -p codewhale-secrets --no-deps --locked PASS
Secret scan GitGuardian (PR check) PASS — 3 commits scanned, no secrets uncovered

Reviewer notes

  1. Two commits in this branch are a rebase, not new work. Upstream moved 426 commits ahead while
    this slice was in progress. The branch is three logical commits on current main. Adopting the
    codewhale-models and codewhale-localization extractions needed four import-path updates, and
    the CmdExportDescription English string was reworded upstream (French unchanged); the tests pin
    the catalog values.
  2. CommandCapabilities has no free bits left. SESSION_EXPORT = 1 << 15 was the last one. The
    next slice needs its own exact-minimum facet, so it must widen the backing storage to u32 (or
    justify reusing a bit, which would break the least-capability invariant this epic is built on). A
    canary test asserts the space is exactly full so this is a deliberate change, not an overflow.
  3. The remaining copy of the transcript is intentional and explained above.
  4. The goldens pin host-derived values (Provider, Model, Mode, the turn route line, and for
    the recorded states the snapshot id and timestamp cell). A future change to the default config or
    the handoff header will fail them by design; they must be re-captured from a verified baseline.

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

Paulo Aboim Pinto

@gitguardian

gitguardian Bot commented Sep 11, 2026

Copy link
Copy Markdown

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@aboimpinto
aboimpinto force-pushed the feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice branch from 54fa34a to 5b0578a Compare September 11, 2026 19:04
Paulo Aboim Pinto added 2 commits September 11, 2026 23:04
…sanitizer

Contract side: `CommandSessionExportContext` with seven operation-granular,
`&self` delegates over contract-owned DTOs, the `SESSION_EXPORT` capability at
bit 15, the envelope slot, and the `export` projections (metadata, typed
transcript with D9 omission variants, semantic restore points, turn handoff).
The raw capability accessor is `#[cfg(test)]` and a canary pins the fact that
bits 0-15 are now fully allocated - the next capability requires widening to
`u32`, which FEAT-026 owns.

Shared sanitizer: the pure ANSI/URL/text/secret redaction primitives move to
`codewhale-secrets` (`redact` + `sanitize`) so portable command helpers have one
acyclic implementation with no TUI, client, or config dependency. The bodies are
relocated verbatim; `config::persistence` re-exports to keep its public API
stable, and `tui::client` / `tui::osc8` delegate so their callers do not drift.

No behaviour change: the moved algorithms, ordering, sensitive-key vocabulary and
results are byte-identical.

Generated with CodeWhale
TUI side of the slice. `SessionExportAdapter` becomes the sole owner of concrete
export machinery - metadata derivation, authoritative/visible-history
projection, semantic restore-point projection, the shared `turn_handoff_markdown`
renderer, clipboard mode/recovery/delivery, and protected destination
resolution/writing - and the envelope exposes it only when `SESSION_EXPORT` is
declared. Hidden reasoning bodies, signatures, and inline/local image payloads
are dropped while the projection is built (D9).

Host services that must outlive the physical move live in
`commands/session_export_host.rs`, outside `groups/session`: the shared
`last-copy.md` recovery writer now reused by `/copy`, and the protected
export-destination resolver/writer. Their algorithms, check order, error text,
and platform behaviour are the baseline implementations relocated unchanged.

The portable handler owns parsing, document rendering, redaction, restore-point
correlation, operation sequencing and every user-visible string. It renders from
an owned projection and redacts JSON in place, so no payload is copied twice.
`/structcopy` only rewires its sanitizer import and stays legacy; its stale
"export seam" prose is corrected.

`/export` keeps its declared order, name, alias, usage, and discovery surface.

Generated with CodeWhale
@aboimpinto
aboimpinto force-pushed the feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice branch from 5b0578a to 8a8de85 Compare September 11, 2026 21:08
@aboimpinto

Copy link
Copy Markdown
Contributor Author

Resolved in 24994fdfd, and noted here because the cause is worth recording.

The single finding was not a real credential. It came from moving the private-key redaction test out of config::persistence into crates/secrets: the original test deliberately assembled its PEM markers at runtime —

// Assemble the PEM markers at runtime so the source file never
// contains a literal private-key header for a scanner to match.
let begin = ["-----BEGIN RSA", " PRIVATE KEY-----"].concat();

— and the rewritten test used the literal header instead. The relocation preserved the algorithm but dropped the convention, which is exactly the class of thing a "verbatim move" is supposed to carry across.

Fixed by restoring the repository's own idiom (runtime assembly of the PEM markers and the token prefix) rather than widening .gitguardian.yml; the config was already correct and adding an ignore would have hidden the next occurrence. The scanner now reports the commits clean.

Paulo Aboim Pinto

@aboimpinto
aboimpinto force-pushed the feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice branch from 8a8de85 to e96013a Compare September 11, 2026 21:38
@aboimpinto

Copy link
Copy Markdown
Contributor Author

Windows failure fixed in e96013ab3Test (windows-latest) failed in "Run tests", and it was mine.

Cause: this PR adds four include_str!() goldens under crates/tui/src/commands/fixtures/ that are compared byte for byte against a document the exporter builds with \n, but I did not declare them in .gitattributes. With * text=auto in effect, a Windows checkout gives those files CRLF, so the comparison failed on a pure \r\n diff. Only Windows can see it; Linux and macOS cannot.

This is precisely the class your own .gitattributes already documents — "fatal for the one that is compared byte for byte" — alongside the telemetry v1.json and Tideline .txt goldens that hit it before. I reproduced it locally by converting a fixture to CRLF: the golden failed with first diff at byte 31, and passed again once reverted.

Fix: declare the new fixtures text eol=lf, matching the existing entries for the other include_str!() inputs. The committed blobs were already LF, so nothing else changes; git ls-files --eol now reports attr/text=auto eol=lf for all four.

Paulo Aboim Pinto

…nd gates

Behaviour preservation is proven rather than claimed. Four goldens were captured
from the pre-refactor implementation at `3f3aa9ed7` by dispatching frozen
fixtures through the baseline public `/export` seam in a scratch worktree:

- authoritative transcript with no restore repository (header, every content
  variant, D9 omission markers, redaction, ANSI stripping),
- the turn handoff,
- the visible-history fallback over a workspace with recorded restore points,
- correlation against recorded restore points (table, newest-first order,
  `tool` snapshots listed but not correlated, the ambiguous double match, and the
  no-match line).

Only genuinely clock-derived fields are normalised (the export stamp, the turn
header stamp, and - for the recorded states - the snapshot id and `Recorded
(UTC)` cell, because `git commit-tree` uses a wall-clock date). A unit test pins
the normaliser so it cannot quietly widen. Everything else is byte-compared.

Also here: the exhaustive `ContextParts` assertion that makes "no unrelated
facet" a compile-time guarantee, the shared host-bound test-support module, the
token-aware extraction audits plus their own unit test, and the boundary gate
extension that keeps `codewhale-secrets` free of any TUI edge.

Carries one unrelated, disclosed repair: a bounded retry for a fork/exec
`ETXTBSY` race in the `wl-paste` regression, kept because the configured
workspace gate was intermittently red without it.

Generated with CodeWhale
@aboimpinto
aboimpinto force-pushed the feat/FEAT-025-adopt-command-shapes-in-tui-session-export-slice branch from e96013a to 54d9261 Compare September 11, 2026 22:21
@aboimpinto

Copy link
Copy Markdown
Contributor Author

macOS failure fixed in 54d9261d8 — again mine, and again only the platform CI could see it.

adapter_preserves_protected_file_write_and_overwrite_refusal and adapter_rejects_directory_destination_and_parent_not_a_directory failed with refusing export through symlink component: /var.

Cause: these two relocated tests call the facet operation write_export_file directly, handing it a raw TempDir path. The old tests in groups/session/export.rs drove /export file … through the command, and the command calls resolve_export_path, which canonicalizes the workspace first. The protected writer deliberately rejects any path with a symlink component — and on macOS /var is a symlink to /private/var, so every TempDir path is rejected. Linux /tmp has no symlink ancestor, so it never reproduced locally.

Fix: the adapter-level tests now build their workspace with a canonical_workspace helper that mirrors what resolve_export_path does for the real command, and the symlink test asserts against the intended symlink rather than passing on macOS for the wrong reason.

I also added protected_writer_requires_a_path_free_of_symlink_ancestors, which creates the symlinked root itself. That turns a macOS-only failure into one a Linux run catches, so this class cannot recur silently.

The production behaviour is unchanged: the symlink rejection is a verbatim relocation of the baseline, and /export file /tmp/x.md being refused on macOS is pre-existing baseline behaviour, not something this PR introduces.

Paulo Aboim Pinto

@aboimpinto
aboimpinto marked this pull request as ready for review September 11, 2026 23:44
@aboimpinto
aboimpinto requested a review from Hmbown as a code owner September 11, 2026 23:44
@aboimpinto

Copy link
Copy Markdown
Contributor Author

Ready for review. All checks are green on 54d9261d8 and the three platform failures CI caught have been fixed — each was mine, and each was invisible to the Linux-only local gate:

Check Failure Cause Fix
GitGuardian 1 secret uncovered the moved private-key test used a literal PEM header; the pre-move test assembled it at runtime so no scanner could match it restored the repository's own idiom (no .gitguardian.yml change; the config was already right)
Test (windows-latest) pure \r\n diff the four new include_str!() goldens were compared byte for byte but not declared in .gitattributes, so * text=auto gave them CRLF declared them text eol=lf, matching the other include_str!() inputs
Test (macos-latest) refusing export through symlink component: /var two relocated adapter tests called write_export_file directly with a raw TempDir path; the pre-move tests drove the command, which canonicalizes the workspace first, and macOS /var is a symlink a canonical_workspace helper mirroring the real command, plus a new test that builds the symlinked root itself so the class now fails on Linux too

Two of the three were repository conventions that the relocation failed to carry over rather than code defects, which is the useful thing to know when reviewing a "structural migration": the algorithm moved correctly, the surrounding discipline did not. Both are now documented in the code and pinned by tests.

Final state:

  • 29 files, +6255 / −1905 against main 447223554; three commits (contract + shared sanitizer, TUI wiring, tests + gates).
  • fmt, Clippy (CI's lint set), nextest 15408/15408, doctests, release build, and the migration/boundary/CI-wiring/locale-parity gates all green locally and in CI.
  • All four baseline-captured goldens still pass unchanged after the rebases, which is the evidence that the migration is behaviour-preserving.
  • No bot review comments remain unaddressed; GitGuardian's notification has a reply.

Please take a look when you have time.

Paulo Aboim Pinto

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

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

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

1 participant