Skip to content

Fix: _strip_ansi regex covers only a subset of CSI escape sequences (#52) - #55

Merged
tbrandenburg merged 2 commits into
mainfrom
fix/issue-52-strip-ansi-regex
Jun 27, 2026
Merged

Fix: _strip_ansi regex covers only a subset of CSI escape sequences (#52)#55
tbrandenburg merged 2 commits into
mainfrom
fix/issue-52-strip-ansi-regex

Conversation

@tbrandenburg

Copy link
Copy Markdown
Owner

Summary

The _strip_ansi helper in tests/test_workflow_to_harness.py only stripped a narrow set of CSI escape-sequence final bytes, making the test suite fragile if Rich emits other valid sequences.

Root Cause

The helper hard-coded [mGKHFJ] instead of matching the full CSI final-byte range the helper name and docstring imply.

Changes

File Change
tests/test_workflow_to_harness.py Broadened _strip_ansi to strip the full CSI range
tests/test_workflow_to_harness.py Added a regression test for cursor-movement CSI sequences

Testing

  • Type check passes
  • Unit tests pass
  • Lint passes
  • Verified the help-contract tests still pass via make qa

Validation

make qa && uv run --locked pytest tests/test_workflow_to_harness.py -k strip_ansi

Issue

Fixes #52

📋 Implementation Details

Implementation followed artifact:

/repo/.agents/issues/issue-52.md

Deviations from plan:

None

Automated implementation from investigation artifact

)

The test helper only stripped a narrow set of CSI final bytes, which made the suite fragile if Rich emitted other valid escape sequences.

Changes:
- Broadened `_strip_ansi` to strip the full CSI range
- Added a regression test for cursor-movement CSI sequences

Fixes #52
@tbrandenburg

Copy link
Copy Markdown
Owner Author

🔍 Automated Code Review

Summary

The fix now matches the broader CSI form expected by the investigation and the regression test covers both cursor movement and private-mode sequences.

Findings

✅ Strengths

  • The helper now strips broader CSI sequences instead of the original narrow final-byte subset.
  • The new regression test exercises both a cursor movement CSI code and a private-mode sequence.

⚠️ Suggestions (non-blocking)

  • None.

🔒 Security

  • No security concerns identified.

Checklist

  • Fix addresses root cause from investigation
  • Code follows codebase patterns
  • Tests cover the change
  • No obvious bugs introduced

Self-reviewed by Claude • Ready for human review

@tbrandenburg

Copy link
Copy Markdown
Owner Author

PR 55 Review

Recommendation

Approve.

Summary

The PR is focused and matches the issue report. The regex expansion in _strip_ansi() correctly covers the CSI forms exercised by Rich, and the new regression test is a good safeguard.

Findings

Low

  1. The new .agents/issues/completed/issue-52.md artifact is tracked in the diff even though the repo’s established issue archive convention appears to live under .ghar/issues/completed/. It does not affect runtime behavior, but it is inconsistent with the existing documentation layout. If this file is intended to be a permanent archive, move it under the repo’s current archive path; otherwise remove it from the PR.

Validation

  • make qa
  • ruff check
  • ruff format --check
  • python -m py_compile
  • pytest ✅ 138 passed
  • uv build

Notes

  • No implementation-report deviations were documented beyond the intended regex/test update.
  • No functional issues were found in tests/test_workflow_to_harness.py.

@tbrandenburg
tbrandenburg merged commit 07e0f0d into main Jun 27, 2026
1 check passed
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.

test: _strip_ansi regex covers only a subset of CSI escape sequences

2 participants