feat(tui): add session export/import command support #1244

Closed
brent.edwards wants to merge 1 commit from feature/m8-tui-session-export-import into master
Member
No description provided.
feat(tui): add session export/import command support
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 19s
CI / helm (pull_request) Successful in 24s
CI / lint (pull_request) Successful in 39s
CI / quality (pull_request) Successful in 3m43s
CI / typecheck (pull_request) Successful in 3m56s
CI / security (pull_request) Successful in 4m7s
CI / unit_tests (pull_request) Successful in 9m11s
CI / docker (pull_request) Successful in 1m20s
CI / e2e_tests (pull_request) Failing after 11m56s
CI / coverage (pull_request) Successful in 12m50s
CI / integration_tests (pull_request) Successful in 25m57s
CI / status-check (pull_request) Failing after 1s
CI / benchmark-regression (pull_request) Successful in 55m1s
fc1c5b65e4
Support /session:export with JSON/Markdown output and /session:import from JSON in the TUI command router, including colon command aliases and safe relative path handling.

Expand Behave and Robot smoke coverage for the new session command flows and update changelog entries.

ISSUES CLOSED: #1004
brent.edwards scheduled this pull request to auto merge when all checks succeed 2026-04-01 19:05:49 +00:00
Owner

🔒 Claimed by pr-reviewer-5. Starting independent code review.

🔒 Claimed by pr-reviewer-5. Starting independent code review.
freemo self-assigned this 2026-04-02 08:06:15 +00:00
Owner

⚠️ Backlog Groomer Notice — Potential Duplicate PR

This PR (#1244 "feat(tui): add session export/import command support") and PR #1269 ("feat(tui): implement session export/import (JSON + Markdown)") both reference Closes #1004 (feat(tui): implement session export/import).

PR #1269 appears to be a more comprehensive implementation (JSON + Markdown formats with BDD tests). Please review whether this PR is superseded by #1269 and consider closing one of them to avoid merge conflicts and duplicate work.

This comment was posted automatically by the backlog groomer.

⚠️ **Backlog Groomer Notice — Potential Duplicate PR** This PR (#1244 "feat(tui): add session export/import command support") and PR #1269 ("feat(tui): implement session export/import (JSON + Markdown)") both reference `Closes #1004` (feat(tui): implement session export/import). PR #1269 appears to be a more comprehensive implementation (JSON + Markdown formats with BDD tests). Please review whether this PR is superseded by #1269 and consider closing one of them to avoid merge conflicts and duplicate work. *This comment was posted automatically by the backlog groomer.*
Owner

⚠️ Merge Conflict Detected — PR #1244 cannot be merged until conflicts are resolved by the implementing agent.

This PR (feature/m8-tui-session-export-import) has merge conflicts with master. Please rebase onto the latest master and resolve all conflicts before this PR can be reviewed and merged.

⚠️ **Merge Conflict Detected** — PR #1244 cannot be merged until conflicts are resolved by the implementing agent. This PR (`feature/m8-tui-session-export-import`) has merge conflicts with `master`. Please rebase onto the latest `master` and resolve all conflicts before this PR can be reviewed and merged.
freemo added this to the v3.7.0 milestone 2026-04-02 08:09:45 +00:00
Owner

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.
freemo requested changes 2026-04-02 16:52:58 +00:00
Dismissed
freemo left a comment

Independent Code Review — PR #1244

Summary

This PR adds session export/import command support to the TUI command router, implementing /session:export (JSON and Markdown formats) and /session:import (from JSON). The code quality is good overall — well-structured, properly typed, with meaningful test coverage. However, there are blocking process issues that prevent merge.


🚫 Blocking Issues (Must Fix)

1. Merge Conflicts (mergeable: false)

The branch has diverged from master and has conflicts in:

  • CHANGELOG.md — Both this branch and master added entries at the top of the "Unreleased" section.
  • robot/tui_smoke.robot — Both this branch and master appended new test cases at the end of the file.

These are textual conflicts that should be straightforward to resolve. Please rebase onto the latest master and resolve conflicts.

2. Empty PR Body

The PR description is completely empty. Per CONTRIBUTING.md, the PR description must:

  • Provide a detailed summary of the changes
  • Include a closing keyword (e.g., Closes #1004)
  • Link the issue as a Forgejo dependency (PR blocks issue)

The commit message is well-written and contains ISSUES CLOSED: #1004, but the PR body itself must also contain this information.


⚠️ Minor Issues (Should Fix)

3. Missing SessionExportError handling in _session_export

The SessionService.export_session() contract declares it can raise both SessionNotFoundError and SessionExportError. Currently only SessionNotFoundError is caught:

try:
    export_data = self.session_service.export_session(session_id)
except SessionNotFoundError:
    return f"Session not found: {session_id}"

If SessionExportError is raised, it will propagate as an unhandled exception in the TUI. Since the command router is the top-level handler for user commands, catching SessionExportError and returning a user-friendly message would be appropriate:

except SessionExportError as exc:
    return f"Session export failed: {exc}"

What Looks Good

  • Commit message follows Conventional Changelog format correctly
  • Single atomic commit — clean history
  • Colon command routing (_split_command) is well-implemented and handles edge cases
  • Path security_resolve_relative_path() properly rejects absolute paths and directory traversal
  • Argument parsing_parse_export_args and _parse_import_args handle --format, --input, positional args, and unknown options correctly
  • Markdown rendering_render_session_markdown() produces clean, structured output
  • Error handling — Comprehensive coverage of file I/O errors, JSON parse errors, and service errors
  • Test coverage — 5 new Behave scenarios covering JSON export, Markdown export, JSON import, invalid JSON import, and colon alias; plus Robot smoke test
  • Type annotations — All new methods are properly typed
  • File length — commands.py stays well under the 500-line limit (~230 lines)
  • SessionService interface — Correctly uses the abstract contract from the domain model
  • session_service: SessionService | None = None — Reasonable optional design for graceful degradation

Duplicate PR Note

PR #1269 (the other PR for issue #1004) was closed without merge, so this PR is the active implementation. The backlog groomer's concern is resolved.


Verdict: REQUEST_CHANGES — Please rebase onto latest master to resolve conflicts, fill in the PR body with a description and Closes #1004, and consider adding SessionExportError handling. The code itself is solid and ready to merge once these items are addressed.

## Independent Code Review — PR #1244 ### Summary This PR adds session export/import command support to the TUI command router, implementing `/session:export` (JSON and Markdown formats) and `/session:import` (from JSON). The code quality is **good overall** — well-structured, properly typed, with meaningful test coverage. However, there are **blocking process issues** that prevent merge. --- ### 🚫 Blocking Issues (Must Fix) #### 1. Merge Conflicts (`mergeable: false`) The branch has diverged from `master` and has conflicts in: - **`CHANGELOG.md`** — Both this branch and master added entries at the top of the "Unreleased" section. - **`robot/tui_smoke.robot`** — Both this branch and master appended new test cases at the end of the file. These are textual conflicts that should be straightforward to resolve. **Please rebase onto the latest `master` and resolve conflicts.** #### 2. Empty PR Body The PR description is completely empty. Per CONTRIBUTING.md, the PR description must: - Provide a detailed summary of the changes - Include a closing keyword (e.g., `Closes #1004`) - Link the issue as a Forgejo dependency (PR blocks issue) The commit message is well-written and contains `ISSUES CLOSED: #1004`, but the PR body itself must also contain this information. --- ### ⚠️ Minor Issues (Should Fix) #### 3. Missing `SessionExportError` handling in `_session_export` The `SessionService.export_session()` contract declares it can raise both `SessionNotFoundError` and `SessionExportError`. Currently only `SessionNotFoundError` is caught: ```python try: export_data = self.session_service.export_session(session_id) except SessionNotFoundError: return f"Session not found: {session_id}" ``` If `SessionExportError` is raised, it will propagate as an unhandled exception in the TUI. Since the command router is the top-level handler for user commands, catching `SessionExportError` and returning a user-friendly message would be appropriate: ```python except SessionExportError as exc: return f"Session export failed: {exc}" ``` --- ### ✅ What Looks Good - **Commit message** follows Conventional Changelog format correctly - **Single atomic commit** — clean history - **Colon command routing** (`_split_command`) is well-implemented and handles edge cases - **Path security** — `_resolve_relative_path()` properly rejects absolute paths and directory traversal - **Argument parsing** — `_parse_export_args` and `_parse_import_args` handle `--format`, `--input`, positional args, and unknown options correctly - **Markdown rendering** — `_render_session_markdown()` produces clean, structured output - **Error handling** — Comprehensive coverage of file I/O errors, JSON parse errors, and service errors - **Test coverage** — 5 new Behave scenarios covering JSON export, Markdown export, JSON import, invalid JSON import, and colon alias; plus Robot smoke test - **Type annotations** — All new methods are properly typed - **File length** — commands.py stays well under the 500-line limit (~230 lines) - **SessionService interface** — Correctly uses the abstract contract from the domain model - **`session_service: SessionService | None = None`** — Reasonable optional design for graceful degradation ### Duplicate PR Note PR #1269 (the other PR for issue #1004) was closed without merge, so this PR is the active implementation. The backlog groomer's concern is resolved. --- **Verdict: REQUEST_CHANGES** — Please rebase onto latest `master` to resolve conflicts, fill in the PR body with a description and `Closes #1004`, and consider adding `SessionExportError` handling. The code itself is solid and ready to merge once these items are addressed.
@ -50,0 +170,4 @@
input_path = Path(arg.split("=", maxsplit=1)[1])
elif arg.startswith("--"):
raise ValueError(f"Unknown option: {arg}")
else:
Owner

The SessionService.export_session() contract can raise SessionExportError in addition to SessionNotFoundError. Consider catching it here to provide a user-friendly error message instead of letting it propagate as an unhandled exception in the TUI:

except SessionNotFoundError:
    return f"Session not found: {session_id}"
except SessionExportError as exc:
    return f"Session export failed: {exc}"
The `SessionService.export_session()` contract can raise `SessionExportError` in addition to `SessionNotFoundError`. Consider catching it here to provide a user-friendly error message instead of letting it propagate as an unhandled exception in the TUI: ```python except SessionNotFoundError: return f"Session not found: {session_id}" except SessionExportError as exc: return f"Session export failed: {exc}" ```
Owner

Review claimed by reviewer pool instance reviewer-pool-2. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-2. Dispatching independent code review.
Owner

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.
freemo requested changes 2026-04-02 17:39:25 +00:00
Dismissed
freemo left a comment

Independent Code Review — PR #1244 (Second Review)

Verdict: REQUEST_CHANGES — PR is superseded; should be closed


🚫 Critical: This PR is superseded by PR #1392 (already merged)

PR #1392 (feat(tui): implement session export/import (JSON + Markdown)) was merged into master at 2026-04-02T17:30:37Z and closed issue #1004. That PR implemented a more comprehensive version of the same feature, including:

  • Session.as_export_markdown() domain method
  • CLI --format flag extension
  • TUI command router export/import handlers
  • 16 Behave scenarios

This PR (#1244) is now obsolete. Merging it would introduce duplicate and conflicting code. The merge conflicts (mergeable: false) are a direct consequence of PR #1392 already modifying the same files.

Recommendation: Close this PR without merge.


Process Issues (would have been blocking even without supersession)

  1. Empty PR body — The PR description is completely empty. Per CONTRIBUTING.md, PRs must have a detailed description explaining the "what" and "why", and must include a closing keyword (Closes #1004). The commit message contains ISSUES CLOSED: #1004 but the PR body itself must also contain this.

  2. Merge conflictsmergeable: false. Conflicts exist in CHANGELOG.md and robot/tui_smoke.robot due to PR #1392 already being merged.


Code Quality Assessment (for the record)

Despite the process issues, the code itself was well-written:

Strengths:

  • Clean, well-structured implementation with proper type annotations
  • _split_command() colon-alias routing is elegant and handles edge cases
  • _resolve_relative_path() provides good security against path traversal
  • _parse_export_args() and _parse_import_args() handle flags, positional args, and unknown options correctly
  • _render_session_markdown() produces clean, structured output
  • Comprehensive error handling for file I/O, JSON parsing, and service errors
  • 5 meaningful Behave scenarios + Robot smoke test
  • Single atomic commit with proper Conventional Changelog format
  • File stays well under 500-line limit

⚠️ Minor issue (would have needed fixing):

  • _session_export() catches SessionNotFoundError but not SessionExportError, while _session_import() catches SessionImportError. This is inconsistent — the export handler should also catch SessionExportError for a user-friendly error message.

Action required: Close this PR. Issue #1004 is already resolved by PR #1392.

## Independent Code Review — PR #1244 (Second Review) ### Verdict: REQUEST_CHANGES — PR is superseded; should be closed --- ### 🚫 Critical: This PR is superseded by PR #1392 (already merged) **PR #1392** (`feat(tui): implement session export/import (JSON + Markdown)`) was merged into `master` at 2026-04-02T17:30:37Z and closed issue #1004. That PR implemented a more comprehensive version of the same feature, including: - `Session.as_export_markdown()` domain method - CLI `--format` flag extension - TUI command router export/import handlers - 16 Behave scenarios **This PR (#1244) is now obsolete.** Merging it would introduce duplicate and conflicting code. The merge conflicts (`mergeable: false`) are a direct consequence of PR #1392 already modifying the same files. **Recommendation: Close this PR without merge.** --- ### Process Issues (would have been blocking even without supersession) 1. **Empty PR body** — The PR description is completely empty. Per CONTRIBUTING.md, PRs must have a detailed description explaining the "what" and "why", and must include a closing keyword (`Closes #1004`). The commit message contains `ISSUES CLOSED: #1004` but the PR body itself must also contain this. 2. **Merge conflicts** — `mergeable: false`. Conflicts exist in `CHANGELOG.md` and `robot/tui_smoke.robot` due to PR #1392 already being merged. --- ### Code Quality Assessment (for the record) Despite the process issues, the code itself was well-written: ✅ **Strengths:** - Clean, well-structured implementation with proper type annotations - `_split_command()` colon-alias routing is elegant and handles edge cases - `_resolve_relative_path()` provides good security against path traversal - `_parse_export_args()` and `_parse_import_args()` handle flags, positional args, and unknown options correctly - `_render_session_markdown()` produces clean, structured output - Comprehensive error handling for file I/O, JSON parsing, and service errors - 5 meaningful Behave scenarios + Robot smoke test - Single atomic commit with proper Conventional Changelog format - File stays well under 500-line limit ⚠️ **Minor issue (would have needed fixing):** - `_session_export()` catches `SessionNotFoundError` but not `SessionExportError`, while `_session_import()` catches `SessionImportError`. This is inconsistent — the export handler should also catch `SessionExportError` for a user-friendly error message. --- **Action required: Close this PR. Issue #1004 is already resolved by PR #1392.**
Owner

Closing this PR as superseded. PR #1392 (feat(tui): implement session export/import (JSON + Markdown)) was already merged into master and closed issue #1004. This PR would introduce duplicate and conflicting code. The code quality was good, but this implementation is no longer needed.

**Closing this PR as superseded.** PR #1392 (`feat(tui): implement session export/import (JSON + Markdown)`) was already merged into `master` and closed issue #1004. This PR would introduce duplicate and conflicting code. The code quality was good, but this implementation is no longer needed.
freemo closed this pull request 2026-04-02 17:39:51 +00:00
freemo left a comment

Independent Code Review — PR #1244 (Superseded)

Verdict: CLOSE WITHOUT MERGE

This PR is superseded by PR #1392, which was already merged into master at 2026-04-02T17:30:37Z. PR #1392 implements the same feature (session export/import with JSON + Markdown support for issue #1004) and has already been reviewed, approved, and merged.

Timeline of events:

  1. PR #1244 (this PR) — original implementation by brent.edwards, opened 2026-04-01
  2. PR #1269 — second implementation attempt, closed without merge
  3. PR #1392 — third implementation, merged 2026-04-02T17:30:37Z, resolving issue #1004

Why this PR cannot be merged:

  1. Superseded — Issue #1004 is already resolved by PR #1392. Merging this would introduce duplicate/conflicting code.
  2. Merge conflictsmergeable: false due to conflicts in CHANGELOG.md and robot/tui_smoke.robot (caused by PR #1392's merge).
  3. Empty PR body — Missing required description, Closes #1004 keyword, and dependency links per CONTRIBUTING.md.

Recommendation:

This PR should be closed without merge. No further action is needed on issue #1004.

## Independent Code Review — PR #1244 (Superseded) ### Verdict: CLOSE WITHOUT MERGE **This PR is superseded by PR #1392**, which was already merged into `master` at 2026-04-02T17:30:37Z. PR #1392 implements the same feature (session export/import with JSON + Markdown support for issue #1004) and has already been reviewed, approved, and merged. ### Timeline of events: 1. **PR #1244** (this PR) — original implementation by `brent.edwards`, opened 2026-04-01 2. **PR #1269** — second implementation attempt, closed without merge 3. **PR #1392** — third implementation, **merged** 2026-04-02T17:30:37Z, resolving issue #1004 ### Why this PR cannot be merged: 1. **Superseded** — Issue #1004 is already resolved by PR #1392. Merging this would introduce duplicate/conflicting code. 2. **Merge conflicts** — `mergeable: false` due to conflicts in `CHANGELOG.md` and `robot/tui_smoke.robot` (caused by PR #1392's merge). 3. **Empty PR body** — Missing required description, `Closes #1004` keyword, and dependency links per CONTRIBUTING.md. ### Recommendation: This PR should be **closed without merge**. No further action is needed on issue #1004.
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 19s
Required
Details
CI / helm (pull_request) Successful in 24s
CI / lint (pull_request) Successful in 39s
Required
Details
CI / quality (pull_request) Successful in 3m43s
Required
Details
CI / typecheck (pull_request) Successful in 3m56s
Required
Details
CI / security (pull_request) Successful in 4m7s
Required
Details
CI / unit_tests (pull_request) Successful in 9m11s
Required
Details
CI / docker (pull_request) Successful in 1m20s
Required
Details
CI / e2e_tests (pull_request) Failing after 11m56s
CI / coverage (pull_request) Successful in 12m50s
Required
Details
CI / integration_tests (pull_request) Successful in 25m57s
Required
Details
CI / status-check (pull_request) Failing after 1s
CI / benchmark-regression (pull_request) Successful in 55m1s

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!1244
No description provided.