Fix parser crash: skip unrecognized lines inside job context#293
Fix parser crash: skip unrecognized lines inside job context#293nitrobass24 merged 2 commits intodevelopfrom
Conversation
Instead of adding specific regex patterns for each PTY line-wrap variant (filename tails, partial eta, mid-word fragments), skip ANY unrecognized line when we're inside a job context (prev_job is set). Lines outside a job context still raise. This eliminates the whack-a-mole pattern of adding regexes for each new fragment type. PTY wrapping can produce arbitrary break points that no fixed set of patterns can anticipate. Fragments seen in production: - "eta:4m [Receiving data]" (wrap before eta) - "ta:4m [Receiving data]" (wrap mid-word) - "tmos.7.1...mkv' at 22283455338 (0%) ..." (filename tail) Add 4 tests: unrecognized-in-job skipped, eta fragment, partial eta fragment, unrecognized-outside-job still raises. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe parser now skips unrecognized lines when parsing inside an active job (prev_job != None), logging a warning instead of raising ValueError. Unrecognized lines outside any job context continue to raise errors. Tests were expanded to cover PTY-wrapped and fragmented progress-line edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/tests/unittests/test_lftp/test_job_status_parser.py`:
- Around line 1589-1621: The file test_job_status_parser.py has formatting
issues flagged by ruff; run ruff format on that test file to fix
whitespace/formatting so CI passes — e.g. format the file containing the test
methods test_unrecognized_line_inside_job_is_skipped,
test_eta_fragment_inside_job_is_skipped,
test_partial_eta_fragment_inside_job_is_skipped, and
test_truly_unrecognized_line_outside_job_raises by running `ruff format` (or
your editor’s ruff integration) on
src/python/tests/unittests/test_lftp/test_job_status_parser.py, then re-run
tests/CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a2c0480-108a-4b1f-a071-8bd2ee6d920c
📒 Files selected for processing (2)
src/python/lftp/job_status_parser.pysrc/python/tests/unittests/test_lftp/test_job_status_parser.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Replaces the whack-a-mole approach to PTY line-wrap fragments with a single rule: skip any unrecognized line when inside a job context.
Problem
PR #290 fixed one specific fragment pattern (filename tails), but production immediately hit two new variants:
eta:4m [Receiving data]— line wraps right before the eta valueta:4m [Receiving data]— line wraps mid-word in "eta"PTY wrapping can break lines at arbitrary byte positions. No finite set of regex patterns can anticipate all possible fragments.
Fix
When
prev_jobis set (we're parsing lines inside a recognized job), skip any unrecognized line with a warning instead of raising. Lines outside any job context still raise to catch genuinely malformed output.This is a safer, more general approach than adding specific patterns for each fragment type.
Tests
test_unrecognized_line_inside_job_is_skipped— garbage inside job is skippedtest_eta_fragment_inside_job_is_skipped—eta:4m [Receiving data]test_partial_eta_fragment_inside_job_is_skipped—ta:4m [Receiving data]test_truly_unrecognized_line_outside_job_raises— garbage with no job context still raises🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests